Add Ethnologue and ROLV Codes to the Create Language Engagement Lookup Modal#1701
Closed
sethmcknight wants to merge 14 commits intodevelopfrom
Closed
Add Ethnologue and ROLV Codes to the Create Language Engagement Lookup Modal#1701sethmcknight wants to merge 14 commits intodevelopfrom
sethmcknight wants to merge 14 commits intodevelopfrom
Conversation
…modal - Identify already engaged Languages for a Project in the CreateLanguageEngagement modal
…Engagement.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…sorting capabilities
…and update LookupField to support initial options
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Show resolved
Hide resolved
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Outdated
Show resolved
Hide resolved
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsx
Outdated
Show resolved
Hide resolved
Contributor
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)
src/components/form/Lookup/LookupField.tsx (1)
18-25:⚠️ Potential issue | 🟡 MinorUse type-only imports for React types.
ComponentTypeandReactNodeare type-only references and should useimport typeto enable tree-shaking and follow the repository's TypeScript standards.🛠️ Suggested fix
-import { - ComponentType, - ReactNode, - useCallback, - useEffect, - useMemo, - useState, -} from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; +import type { ComponentType, ReactNode } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/Lookup/LookupField.tsx` around lines 18 - 25, The import currently brings ComponentType and ReactNode as value imports; change them to type-only imports to follow TS standards and enable tree-shaking: update the import in LookupField.tsx so that ComponentType and ReactNode use "import type" (leaving hooks like useCallback, useEffect, useMemo, useState as regular imports) and ensure any other occurrences of ComponentType or ReactNode in this file remain type-only references.src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (1)
15-22:⚠️ Potential issue | 🟡 MinorUse
import typefor type-only imports.
LanguageLookupItemandCreateLanguageEngagementMutationare used only as type annotations and should be imported withimport type.🛠️ Suggested fix
-import { - LanguageField, - LanguageLookupItem, -} from '../../../../components/form/Lookup'; +import { LanguageField } from '../../../../components/form/Lookup'; +import type { LanguageLookupItem } from '../../../../components/form/Lookup'; -import { - CreateLanguageEngagementDocument, - CreateLanguageEngagementMutation, -} from './CreateLanguageEngagement.graphql'; +import { CreateLanguageEngagementDocument } from './CreateLanguageEngagement.graphql'; +import type { CreateLanguageEngagementMutation } from './CreateLanguageEngagement.graphql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 15 - 22, The imports include type-only symbols that should use `import type`; update the first import so `LanguageLookupItem` is imported with `import type` (keep `LanguageField` as a value import), and update the second import so `CreateLanguageEngagementMutation` is imported with `import type` while keeping `CreateLanguageEngagementDocument` as a value import; ensure syntax is `import { LanguageField } from '...'; import type { LanguageLookupItem } from '...';` and similarly split the GraphQL import into a value import for CreateLanguageEngagementDocument and a type import for CreateLanguageEngagementMutation.
🧹 Nitpick comments (1)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (1)
41-90: Prefersx/styledfor new styling blocks.This introduces new
makeStylesusage; prefersx/styledfor new code unless there’s a strong reason to keep legacy styling.As per coding guidelines: Prefer
sxprop for new code;makeStylesis legacy but acceptable in existing files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 41 - 90, This file adds a new makeStyles block (useStyles) which is legacy; replace it with MUI v5 styling by removing useStyles and converting the rule set for the classes (columnHeader, columnHeaderName, columnHeaderCode, optionRow, optionName, optionCode, helperRow, helperKey) into either sx props on the corresponding components or into styled components (using styled() or the sx callback to access theme.palette/spacing). Update the components that reference useStyles/classes to apply the equivalent sx objects or styled components and ensure theme values (palette, spacing) are accessed via the theme in sx or useTheme so visual parity is preserved. Ensure no leftover makeStyles/imports remain.
🤖 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/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 96-102: Create a named props interface FormContentProps above the
FormContent component instead of the inline annotation; move the current inline
type ({ engagedLanguageIds: readonly string[]; sortComparator: (a:
LanguageLookupItem, b: LanguageLookupItem) => number }) into that interface and
then update the FormContent signature to accept (props: FormContentProps) or ({
engagedLanguageIds, sortComparator }: FormContentProps). Ensure the interface
references engagedLanguageIds, sortComparator and the LanguageLookupItem type
exactly as used in the component.
- Around line 104-170: The code dereferences values.engagement.languageId on
initial render causing a TypeError; update the use of currentLanguage so it is
safely guarded and nested properties are optional. Change the assignment to use
optional chaining/default (e.g. const currentLanguage =
values.engagement?.languageId ?? null) and update the helperText and any places
that access currentLanguage.ethnologue.code.value or
currentLanguage.registryOfLanguageVarietiesCode.value to use optional chaining
and fallbacks (e.g. currentLanguage?.ethnologue?.code?.value ?? '-' and
currentLanguage?.registryOfLanguageVarietiesCode?.value ?? '-'). Ensure
getOptionDisabled/renderOptionContent logic that reads engagedLanguageIds
remains unchanged.
---
Outside diff comments:
In `@src/components/form/Lookup/LookupField.tsx`:
- Around line 18-25: The import currently brings ComponentType and ReactNode as
value imports; change them to type-only imports to follow TS standards and
enable tree-shaking: update the import in LookupField.tsx so that ComponentType
and ReactNode use "import type" (leaving hooks like useCallback, useEffect,
useMemo, useState as regular imports) and ensure any other occurrences of
ComponentType or ReactNode in this file remain type-only references.
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 15-22: The imports include type-only symbols that should use
`import type`; update the first import so `LanguageLookupItem` is imported with
`import type` (keep `LanguageField` as a value import), and update the second
import so `CreateLanguageEngagementMutation` is imported with `import type`
while keeping `CreateLanguageEngagementDocument` as a value import; ensure
syntax is `import { LanguageField } from '...'; import type { LanguageLookupItem
} from '...';` and similarly split the GraphQL import into a value import for
CreateLanguageEngagementDocument and a type import for
CreateLanguageEngagementMutation.
---
Nitpick comments:
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 41-90: This file adds a new makeStyles block (useStyles) which is
legacy; replace it with MUI v5 styling by removing useStyles and converting the
rule set for the classes (columnHeader, columnHeaderName, columnHeaderCode,
optionRow, optionName, optionCode, helperRow, helperKey) into either sx props on
the corresponding components or into styled components (using styled() or the sx
callback to access theme.palette/spacing). Update the components that reference
useStyles/classes to apply the equivalent sx objects or styled components and
ensure theme values (palette, spacing) are accessed via the theme in sx or
useTheme so visual parity is preserved. Ensure no leftover makeStyles/imports
remain.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/form/Lookup/LookupField.test.tsxsrc/components/form/Lookup/LookupField.tsxsrc/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsxsrc/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsxsrc/scenes/Projects/Overview/ProjectOverview.tsx
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Outdated
Show resolved
Hide resolved
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Show resolved
Hide resolved
…ve clarity and modularity of logic; add optional chaining for languageId.
Contributor
There was a problem hiding this comment.
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 (2)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (1)
15-22:⚠️ Potential issue | 🟡 MinorUse type-only imports for lookup and mutation types.
LanguageLookupItemandCreateLanguageEngagementMutationare used only in type positions; import them withimport typeto align with repo rules.🧹 Suggested change
-import { - LanguageField, - LanguageLookupItem, -} from '../../../../components/form/Lookup'; +import { LanguageField } from '../../../../components/form/Lookup'; +import type { LanguageLookupItem } from '../../../../components/form/Lookup'; import { CreateLanguageEngagementDocument, - CreateLanguageEngagementMutation, } from './CreateLanguageEngagement.graphql'; +import type { CreateLanguageEngagementMutation } from './CreateLanguageEngagement.graphql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 15 - 22, The imports include non-type imports for symbols only used as types; change the imports to use type-only imports for LanguageLookupItem and CreateLanguageEngagementMutation so the compiler/tree-shaker knows they are types—update the Lookup import to `import type { LanguageLookupItem }` (keeping LanguageField as a value import) and update the GraphQL import to `import type { CreateLanguageEngagementMutation }` while retaining the runtime CreateLanguageEngagementDocument import.src/components/form/Lookup/LookupField.tsx (1)
18-25:⚠️ Potential issue | 🟡 MinorUse type-only imports for React types.
ComponentTypeandReactNodeare used exclusively as type annotations (for property types, function return types, and generic parameters) and never at runtime. Per coding guidelines, they must useimport typesyntax.🧹 Suggested change
-import { - ComponentType, - ReactNode, - useCallback, - useEffect, - useMemo, - useState, -} from 'react'; +import { useCallback, useEffect, useMemo, useState } from 'react'; +import type { ComponentType, ReactNode } from 'react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/form/Lookup/LookupField.tsx` around lines 18 - 25, The import currently brings ComponentType and ReactNode as runtime imports; change those to type-only imports by using "import type" for ComponentType and ReactNode while leaving hooks (useCallback, useEffect, useMemo, useState) as regular imports so only types are erased at emit; update the import statement in LookupField.tsx to import type ComponentType and ReactNode and keep the hook imports unchanged.
♻️ Duplicate comments (1)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (1)
26-30:⚠️ Potential issue | 🔴 CriticalGuard form values and nested codes before access.
On initial render,
values.engagementcan be undefined, and nested code objects can be null. This can throw before a selection is made.🛠️ Suggested fix
- const currentLanguage = values.engagement.languageId ?? null; + const currentLanguage = values.engagement?.languageId ?? null; ... - {option.name.value ?? option.displayName.value} + {option.name.value ?? option.displayName?.value ?? '-'} ... - {option.ethnologue.code.value ?? '-'} + {option.ethnologue?.code?.value ?? '-'} ... - {option.registryOfLanguageVarietiesCode.value ?? '-'} + {option.registryOfLanguageVarietiesCode?.value ?? '-'} ... - {currentLanguage?.ethnologue.code.value ?? '-'} + {currentLanguage?.ethnologue?.code?.value ?? '-'} ... - {currentLanguage?.registryOfLanguageVarietiesCode.value ?? '-'} + {currentLanguage?.registryOfLanguageVarietiesCode?.value ?? '-'}As per coding guidelines, "Use strict TypeScript: ... use optional chaining (
?.) or type guards for safe property access, never assume properties exist".Also applies to: 107-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 26 - 30, The form values interface and usages assume values.engagement always exists and access nested fields like languageId/code directly; make engagement optional in CreateLanguageEngagementFormValues (e.g., engagement?: { languageId?: LanguageLookupItem }) and update all places in the CreateLanguageEngagement component where values.engagement or nested objects are read (including render logic and submit handlers) to use optional chaining or explicit guards (values.engagement?.languageId, languageId?.someProp, or if (values.engagement && values.engagement.languageId) return ...) so no property access occurs on undefined/null; apply the same guarding pattern to every access of values.engagement and nested codes referenced in the component (the blocks around the earlier referenced usage).
🤖 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/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 145-154: The custom PaperComponent is dropping Autocomplete's
props (className, ref, role, onMouseDown, etc.); change the lambda in
CreateLanguageEngagement.tsx from PaperComponent={({ children }) => (...) } to
accept the full props (e.g. paperProps), pull out children and spread the rest
onto the Paper: const { children, ...rest } = paperProps; return <Paper
{...rest}>...{children}</Paper>; this preserves forwarded ref and other behavior
while keeping your header Box and children rendering.
---
Outside diff comments:
In `@src/components/form/Lookup/LookupField.tsx`:
- Around line 18-25: The import currently brings ComponentType and ReactNode as
runtime imports; change those to type-only imports by using "import type" for
ComponentType and ReactNode while leaving hooks (useCallback, useEffect,
useMemo, useState) as regular imports so only types are erased at emit; update
the import statement in LookupField.tsx to import type ComponentType and
ReactNode and keep the hook imports unchanged.
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 15-22: The imports include non-type imports for symbols only used
as types; change the imports to use type-only imports for LanguageLookupItem and
CreateLanguageEngagementMutation so the compiler/tree-shaker knows they are
types—update the Lookup import to `import type { LanguageLookupItem }` (keeping
LanguageField as a value import) and update the GraphQL import to `import type {
CreateLanguageEngagementMutation }` while retaining the runtime
CreateLanguageEngagementDocument import.
---
Duplicate comments:
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 26-30: The form values interface and usages assume
values.engagement always exists and access nested fields like languageId/code
directly; make engagement optional in CreateLanguageEngagementFormValues (e.g.,
engagement?: { languageId?: LanguageLookupItem }) and update all places in the
CreateLanguageEngagement component where values.engagement or nested objects are
read (including render logic and submit handlers) to use optional chaining or
explicit guards (values.engagement?.languageId, languageId?.someProp, or if
(values.engagement && values.engagement.languageId) return ...) so no property
access occurs on undefined/null; apply the same guarding pattern to every access
of values.engagement and nested codes referenced in the component (the blocks
around the earlier referenced usage).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/form/Lookup/LookupField.test.tsxsrc/components/form/Lookup/LookupField.tsxsrc/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Outdated
Show resolved
Hide resolved
…teSortComparator and createGetOptionDisabled; enhance clarity and maintainability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (1)
224-255: No error handling insubmit— unhandled API errors won't surface to the user.The
createEngagementcall isn't wrapped in try/catch andhandleFormErrorisn't used, so mutation failures will be swallowed silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 224 - 255, The submit function currently awaits createEngagement without error handling; wrap the createEngagement call inside a try/catch in the submit function and on error call the existing handleFormError(error) (and return or rethrow as appropriate) so API errors are surfaced to the user; locate the submit function in CreateLanguageEngagement.tsx and ensure the catch handles the thrown error from createEngagement and delegates to handleFormError before exiting.
🧹 Nitpick comments (5)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (3)
135-158:renderOptionContentis redefined on every render — consideruseCallback.Like
getOptionDisabled, this closure is recreated on every form-value change. Stabilising it withuseCallback([engagedLanguageIds, classes])avoids unnecessary prop churn intoLanguageField.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 135 - 158, renderOptionContent is being recreated on every render causing prop churn to LanguageField; wrap it in React.useCallback and memoize it with dependencies [engagedLanguageIds, classes] so it is stable across renders, similar to getOptionDisabled. Locate the renderOptionContent function and replace its inline closure with a useCallback hook that returns the same JSX, ensuring engagedLanguageIds and classes are included in the dependency array and that the returned value semantics remain unchanged for disabled tooltip handling.
17-18: Useimport typeforLanguageLookupItemsince it's used only as a type.
LanguageLookupItemappears only in type-position usages (function parameters, interface fields). Per guidelines, usetypeimports for type-only references.♻️ Proposed fix
-import { - LanguageField, - LanguageLookupItem, -} from '../../../../components/form/Lookup'; +import { LanguageField } from '../../../../components/form/Lookup'; +import type { LanguageLookupItem } from '../../../../components/form/Lookup';As per coding guidelines: "Use
typeimports for type-only references."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 17 - 18, The import of LanguageLookupItem should be a type-only import; change the import that currently brings in LanguageLookupItem from '../../../../components/form/Lookup' to an `import type` so TypeScript treats it as a type-only reference—update usages in CreateLanguageEngagement (e.g., prop types or handlers referencing LanguageLookupItem) to keep runtime imports unchanged and satisfy the type-only guideline.
188-188:createGetOptionDisabled(engagedLanguageIds)creates a new function reference on every render.
FormContentsubscribes to{ values: true }, so it re-renders on every keystroke. This call produces a fresh function instance each time, which will break any memoization insideLanguageField.♻️ Proposed fix
+ const getOptionDisabled = useMemo( + () => createGetOptionDisabled(engagedLanguageIds), + [engagedLanguageIds] + ); + return ( <> <SubmitError /> <LanguageField ... - getOptionDisabled={createGetOptionDisabled(engagedLanguageIds)} + getOptionDisabled={getOptionDisabled}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` at line 188, The current inline call to createGetOptionDisabled(engagedLanguageIds) on each render creates a new function reference every time (breaking memoization in LanguageField); fix by memoizing the created function and passing that stable reference into FormContent: compute getOptionDisabled once with useMemo (or move creation outside render) keyed on engagedLanguageIds and then pass getOptionDisabled to FormContent instead of calling createGetOptionDisabled inline; reference createGetOptionDisabled and where you pass getOptionDisabled into FormContent/LanguageField so reviewers can find the change.src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsx (2)
303-312: TheFormwrapper inrenderHelperTextadds no value.
HelperTextPreviewreceiveslanguagedirectly as a prop and never callsuseFormStateor any other Final Form hook. Wrapping it in<Form>doesn't exercise a realistic render path — it just adds noise. A plainrender(<HelperTextPreview language={lang} />)is clearer.♻️ Proposed simplification
-const renderHelperText = (language: LanguageLookupItem) => - render( - <Form - onSubmit={() => undefined} - initialValues={{ engagement: { languageId: language } }} - render={() => <HelperTextPreview language={language} />} - /> - ); +const renderHelperText = (language: LanguageLookupItem) => + render(<HelperTextPreview language={language} />);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsx` around lines 303 - 312, The test helper renderHelperText unnecessarily wraps HelperTextPreview in a Final Form <Form>; change renderHelperText to directly render HelperTextPreview with the language prop (i.e., return render(<HelperTextPreview language={language} />)), remove the unused Form-related initialValues/onSubmit, and delete any now-unused Form import so the test is clearer and no longer performs an irrelevant Final Form render.
157-281:renderOptionContenttests exercise a re-implementation, not the production function.
makeRenderOptionContentis a local copy of the productionrenderOptionContentlogic written specifically for this test. If the actualrenderOptionContentinsideFormContentchanges (different tooltip title, different element structure, different null-fallback logic), these tests still pass — they provide no regression protection for the real code.The same applies to
HelperTextPreview(lines 292–301) andColumnHeader(lines 364–370), which re-implement the productionhelperTextJSX andPaperComponentheader row, respectively.Consider one of these approaches instead:
- Export
renderOptionContent(or amakeRenderOptionContentfactory) directly fromCreateLanguageEngagement.tsxso it can be imported and tested here without re-implementation.- Write a shallow integration test that renders
FormContentwith mocked Apollo/routing providers and asserts on the rendered output.Option 1 is lower friction and keeps the tests focused on pure logic.
♻️ Sketch for option 1 (export the factory)
In
CreateLanguageEngagement.tsx, extract and export the factory:// Exported for unit testing export const makeRenderOptionContent = (engagedLanguageIds: readonly string[], classes: ReturnType<typeof useStyles>['classes']) => (option: LanguageLookupItem) => { /* ... existing logic ... */ };Then in the test:
import { makeRenderOptionContent } from './CreateLanguageEngagement'; // use it directly — no re-implementation needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsx` around lines 157 - 281, Tests re-implement production rendering helpers (makeRenderOptionContent, HelperTextPreview, ColumnHeader) instead of importing them, so they won't catch regressions; export the real factory or helpers from CreateLanguageEngagement.tsx (e.g., export makeRenderOptionContent or renderOptionContent and helperText / PaperComponent header) and update the test to import and call those exported symbols (or alternatively replace the unit tests with a shallow integration test rendering FormContent with mocked providers); ensure the test references the exported function names (makeRenderOptionContent, HelperTextPreview, ColumnHeader or renderOptionContent, FormContent) rather than a local copy.
🤖 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/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 73-86: The styles use magic numbers (fontSize '0.7rem' and width
52) in columnHeaderCode and the matching optionCode, so add inline comments next
to these values (e.g., "// fixed width matching columnHeaderCode" for the width
and a short note for the fontSize) to document intent and that they must stay in
sync; update both columnHeaderCode and optionCode references so future
maintainers know these values are deliberately paired.
---
Duplicate comments:
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 224-255: The submit function currently awaits createEngagement
without error handling; wrap the createEngagement call inside a try/catch in the
submit function and on error call the existing handleFormError(error) (and
return or rethrow as appropriate) so API errors are surfaced to the user; locate
the submit function in CreateLanguageEngagement.tsx and ensure the catch handles
the thrown error from createEngagement and delegates to handleFormError before
exiting.
---
Nitpick comments:
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsx`:
- Around line 303-312: The test helper renderHelperText unnecessarily wraps
HelperTextPreview in a Final Form <Form>; change renderHelperText to directly
render HelperTextPreview with the language prop (i.e., return
render(<HelperTextPreview language={language} />)), remove the unused
Form-related initialValues/onSubmit, and delete any now-unused Form import so
the test is clearer and no longer performs an irrelevant Final Form render.
- Around line 157-281: Tests re-implement production rendering helpers
(makeRenderOptionContent, HelperTextPreview, ColumnHeader) instead of importing
them, so they won't catch regressions; export the real factory or helpers from
CreateLanguageEngagement.tsx (e.g., export makeRenderOptionContent or
renderOptionContent and helperText / PaperComponent header) and update the test
to import and call those exported symbols (or alternatively replace the unit
tests with a shallow integration test rendering FormContent with mocked
providers); ensure the test references the exported function names
(makeRenderOptionContent, HelperTextPreview, ColumnHeader or
renderOptionContent, FormContent) rather than a local copy.
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 135-158: renderOptionContent is being recreated on every render
causing prop churn to LanguageField; wrap it in React.useCallback and memoize it
with dependencies [engagedLanguageIds, classes] so it is stable across renders,
similar to getOptionDisabled. Locate the renderOptionContent function and
replace its inline closure with a useCallback hook that returns the same JSX,
ensuring engagedLanguageIds and classes are included in the dependency array and
that the returned value semantics remain unchanged for disabled tooltip
handling.
- Around line 17-18: The import of LanguageLookupItem should be a type-only
import; change the import that currently brings in LanguageLookupItem from
'../../../../components/form/Lookup' to an `import type` so TypeScript treats it
as a type-only reference—update usages in CreateLanguageEngagement (e.g., prop
types or handlers referencing LanguageLookupItem) to keep runtime imports
unchanged and satisfy the type-only guideline.
- Line 188: The current inline call to
createGetOptionDisabled(engagedLanguageIds) on each render creates a new
function reference every time (breaking memoization in LanguageField); fix by
memoizing the created function and passing that stable reference into
FormContent: compute getOptionDisabled once with useMemo (or move creation
outside render) keyed on engagedLanguageIds and then pass getOptionDisabled to
FormContent instead of calling createGetOptionDisabled inline; reference
createGetOptionDisabled and where you pass getOptionDisabled into
FormContent/LanguageField so reviewers can find the change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.test.tsxsrc/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Show resolved
Hide resolved
…reateLanguageEngagement component.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx (2)
78-103: Tag inline comments with// ai <tag>per repo convention.The new inline comments are untagged; add an AI tag (e.g.,
// ai design-alignment) to keep comment searchability consistent.✍️ Suggested fix
- width: 52, // must match optionCode width for column alignment - fontSize: '0.7rem', // compact size for column header labels + width: 52, // ai design-alignment: must match optionCode width for column alignment + fontSize: '0.7rem', // ai design-alignment: compact size for column header labels ... - width: 52, // must match columnHeaderCode width for column alignment + width: 52, // ai design-alignment: must match columnHeaderCode width for column alignmentAs per coding guidelines: "Use tagged comments
// ai tagfor code searchability:example,edge-case,best-practice,anti-pattern,todo,workaround,performance,security,test,design-alignment,type-safety."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 78 - 103, The inline comments inside the style object properties (e.g., the comments in columnHeaderCode and optionCode like "// must match optionCode width for column alignment" and "// compact size for column header labels") are missing the required AI tag; update each such inline comment in the CreateLanguageEngagement styles (columnHeaderCode, optionCode, optionRow, optionName if applicable) to include a repo-convention tag suffix (for example "// ai design-alignment") so comments are searchable and conform to "ai <tag>" formats.
19-22: Useimport typeforCreateLanguageEngagementMutation.This type is used only as a type annotation (line 244) and should be imported with the
typekeyword to keep value imports lean and align with repo TypeScript style.♻️ Suggested fix
-import { - CreateLanguageEngagementDocument, - CreateLanguageEngagementMutation, -} from './CreateLanguageEngagement.graphql'; +import { CreateLanguageEngagementDocument } from './CreateLanguageEngagement.graphql'; +import type { CreateLanguageEngagementMutation } from './CreateLanguageEngagement.graphql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx` around lines 19 - 22, The import of CreateLanguageEngagementMutation is used only for typing; change the import to a type-only import by prefixing it with the TypeScript `type` modifier so the statement imports CreateLanguageEngagementDocument as a value and CreateLanguageEngagementMutation as a type (e.g., use `import { CreateLanguageEngagementDocument, type CreateLanguageEngagementMutation } from './CreateLanguageEngagement.graphql';`) to keep value imports lean and follow the repo TS style.
🤖 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/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 224-227: Replace the unsafe non-null assertions in the submit
handler by adding an explicit guard that verifies engagement and
engagement.languageId before using them: in the submit function (submit) check
if engagement?.languageId is present and handle the missing case (e.g., set a
form error, show a validation message, or return early) instead of using
engagement!.languageId! so the type is narrowed safely and no runtime exception
can occur.
- Around line 135-146: renderOptionContent and the helper-text rendering access
nested fields like option.ethnologue.code.value and
option.registryOfLanguageVarietiesCode.value directly, which can throw when
those objects are null; update renderOptionContent and the corresponding helper
text rendering (the other block referenced around the same component) to use
optional chaining (e.g., option.ethnologue?.code?.value and
option.registryOfLanguageVarietiesCode?.value) and provide a safe fallback
(e.g., '–' or '-') via nullish coalescing so missing ETH/ROLV data won't crash
the UI; ensure all similar nested accesses in CreateLanguageEngagement,
including any helperText or displayName fallbacks, are changed consistently.
---
Nitpick comments:
In
`@src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx`:
- Around line 78-103: The inline comments inside the style object properties
(e.g., the comments in columnHeaderCode and optionCode like "// must match
optionCode width for column alignment" and "// compact size for column header
labels") are missing the required AI tag; update each such inline comment in the
CreateLanguageEngagement styles (columnHeaderCode, optionCode, optionRow,
optionName if applicable) to include a repo-convention tag suffix (for example
"// ai design-alignment") so comments are searchable and conform to "ai <tag>"
formats.
- Around line 19-22: The import of CreateLanguageEngagementMutation is used only
for typing; change the import to a type-only import by prefixing it with the
TypeScript `type` modifier so the statement imports
CreateLanguageEngagementDocument as a value and CreateLanguageEngagementMutation
as a type (e.g., use `import { CreateLanguageEngagementDocument, type
CreateLanguageEngagementMutation } from './CreateLanguageEngagement.graphql';`)
to keep value imports lean and follow the repo TS style.
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Show resolved
Hide resolved
src/scenes/Engagement/LanguageEngagement/Create/CreateLanguageEngagement.tsx
Outdated
Show resolved
Hide resolved
…and alignment consistency in styles
sethmcknight
changed the title
sethmcknight
added
the
🚧 Blocked
Contributor
|
Resolved by #1791 |
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
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.
Overview
Enhanced the Create Language Engagement modal to improve language discovery and prevent duplicate engagements by displaying language classification codes and disabling languages that are already engaged on the project.
Improves language selection UX by making language codes visible at selection time.
Changes
Display language codes in dropdown: Each language option now shows:
Language name
Ethnologue code (ETH)
Registry of Language Varieties code (ROLV)
Column headers: Added labeled column headers (Name, ETH, ROLV) for clarity in the dropdown options list
Prevent duplicate engagements:
Languages already engaged on the project are disabled in the dropdown
Disabled options display a tooltip: "Already added to this project"
Already-engaged languages are sorted to the bottom of the list for better UX
Selected language helper text: When a language is selected, its ETH and ROLV codes appear below the field for quick reference
Code clarity: Renamed form field structure from language to engagement throughout the component for consistency with modal intent
Component refactor: Extracted form content into a separate FormContent component for better code organization and reusability