Add interlinearizer feature with XML parsing and web view integration #6
Conversation
- Introduced `interlinearizer.web-view.tsx` and `interlinearizer.web-view.scss` for rendering interlinear data. - Implemented `InterlinearXmlParser` for parsing interlinear XML data. - Updated `package.json` and `package-lock.json` to include `fast-xml-parser` dependency. - Added new types for interlinear data in `paranext-extension-template.d.ts`. - Included sample interlinear XML data in `test-data/Interlinear_en_MAT.xml`. - Enhanced `main.ts` to register the new web view provider for the interlinearizer. - Updated `cspell.json` with new terms related to interlinearization.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new InterlinearXmlParser and types, a React WebView component and Tailwind-enabled SCSS, extensive Jest test infrastructure with mocks and test data, modifies activation/provider lifecycle in main.ts, adds fast-xml-parser runtime dependency and test tooling, and introduces a GitHub Actions test workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as Extension (main.ts)
participant PAPI as `@papi/backend`
participant Provider as WebViewProvider
participant WebViewSys as WebView System
Note over Ext,PAPI: Activation & provider registration
Ext->>PAPI: activate(context)
Ext->>Provider: construct provider (getWebView)
Ext->>PAPI: registerWebViewProvider(mainWebViewType, Provider)
PAPI-->>Ext: registration disposable
Ext->>Ext: store disposable in context
Ext->>PAPI: openWebView(mainWebViewType)
PAPI->>Provider: getWebView(mainWebViewType)
Provider->>WebViewSys: return WebViewDefinition (title, content, styles)
WebViewSys->>WebViewSys: initialize/render content
Ext->>PAPI: logger.info('finished activating')
Note over Ext,PAPI: Deactivation
Ext->>Ext: deactivate()
Ext->>Ext: run unsubscribers / dispose
Ext-->>PAPI: return boolean
sequenceDiagram
participant Component as InterlinearWebView
participant Parser as InterlinearXmlParser
participant Renderer as ReactRenderer
Component->>Component: useMemo create parser
Component->>Parser: parse(bundled XML)
alt Success
Parser->>Component: return InterlinearData
Component->>Renderer: render parsed JSON view
else Failure
Parser->>Component: throw Error
Component->>Renderer: render error block
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 38: Update the vulnerable dependency declaration for fast-xml-parser by
changing the version string in package.json from "fast-xml-parser": "^5.3.3" to
"fast-xml-parser": "^5.3.4" (reference: the dependency key fast-xml-parser in
package.json), then regenerate your lockfile (npm install or yarn install) to
update package-lock.json / yarn.lock and run the test suite/build to ensure no
regressions.
In `@src/main.ts`:
- Line 38: The call to papi.webViews.openWebView(mainWebViewType, undefined, {
existingId: '?' }) returns a Promise that is currently unhandled; modify the
call in main.ts so the returned Promise is either awaited inside an async
context (await papi.webViews.openWebView(...)) or attach a .catch(...) handler
to handle errors, ensuring any thrown error is logged or handled; locate the
invocation by the identifier openWebView and the mainWebViewType/existingId
argument to update it.
🧹 Nitpick comments (3)
src/parsers/interlinearXmlParser.ts (2)
118-140: Consider consistent error handling between clusters and punctuations.This function silently skips punctuations with missing Range elements (lines 122-127), while
extractClustersFromVersethrows an error for the same condition. If this difference is intentional (punctuations being optional/non-critical), consider adding a brief comment explaining why punctuations are filtered rather than validated.
169-170: Edge case: EmptylexemesIdproduces IDs starting with/.When a cluster has no lexemes,
lexemesIdis an empty string, producing IDs like"/5-10". If these IDs are used as keys or for lookups, the leading slash may cause unexpected behavior.Consider whether clusters without lexemes are valid, and if so, whether the ID format should handle this case differently.
src/interlinearizer.web-view.tsx (1)
46-53: Minor: Redundant!parseErrorcheck.Given the discriminated union
ParseResult, whenparsedis truthy,parseErroris alwaysundefined. The!parseErrorcheck is technically redundant but harmless—it may help TypeScript narrowing or serve as defensive coding.
- Bump `fast-xml-parser` version to `5.3.4` in `package.json` and `package-lock.json`. - Upgrade `lockfileVersion` in `package-lock.json` to `3`. - Improve README to clarify the role of `package.json` and `package-lock.json`, and add details about the new `test-data/` directory for sample interlinear XML. - Add comments in `src/main.ts` and `src/interlinearizer.web-view.tsx` for better code documentation.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/interlinearizer.web-view.tsx`:
- Around line 18-19: Add a TypeScript global augmentation so the assignment to
globalThis.webViewComponent is typed: create a .d.ts file and declare a global
block that adds a var webViewComponent typed as a function returning a
JSX.Element (or the actual component return type), e.g. declare global { var
webViewComponent: () => JSX.Element; } and ensure the .d.ts is included in the
TS build (or referenced) so the compiler recognizes the global.
In `@src/main.ts`:
- Around line 52-61: The WebView is opened before its provider registration
completes, causing a race; await the registration promise before calling
papi.webViews.openWebView and before adding the provider to
context.registrations. Concretely, wait for mainWebViewProviderPromise (result
of papi.webViewProviders.registerWebViewProvider with
mainWebViewType/mainWebViewProvider), add the awaited registration to
context.registrations, then call papi.webViews.openWebView(mainWebViewType, ...)
and keep the existing error handling.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec made 3 comments.
Reviewable status: 0 of 10 files reviewed, 3 unresolved discussions (waiting on @alex-rawlings-yyc and @jasonleenaylor).
src/parsers/interlinearXmlParser.ts line 9 at r3 (raw file):
PunctuationData, StringRange, } from 'paranext-extension-template';
This file has several references to paranext-extension-template that need updating.
src/main.ts line 15 at r3 (raw file):
* opening the WebView from the platform. */ const mainWebViewType = 'paranextExtensionTemplate.interlinearizer';
'paranextExtensionTemplate.interlinearizer' -> 'interlinearizer.<something like mainWebView>'
src/interlinearizer.web-view.tsx line 2 at r3 (raw file):
import { useMemo } from 'react'; import type { InterlinearData } from 'paranext-extension-template';
Need to update 'paranext-extension-template' to 'interlinearizer'.
- Changed import source for `InterlinearData` and other types from `paranext-extension-template` to `interlinearizer` in `src/interlinearizer.web-view.tsx` and `src/parsers/interlinearXmlParser.ts`. - Updated the `mainWebViewType` constant in `src/main.ts` to reflect the new naming convention for the interlinearizer web view.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/parsers/interlinearXmlParser.ts`:
- Around line 122-143: In extractPunctuationsFromVerse, the current guard only
checks for undefined on range fields and may pass non-numeric values into the
returned TextRange; update the filter to validate/coerce the Range values
(el.Range['@_Index'] and el.Range['@_Length']) are finite numbers (e.g., use
Number(...) or parseInt and Number.isFinite / !Number.isNaN checks) and skip
entries that fail validation, then in the map emit Index and Length as numeric
values (not raw strings) so TextRange satisfies the expected numeric StringRange
shape.
- Around line 155-187: In extractClustersFromVerse, rangeElement['@_Index'] and
['@_Length'] are only checked for undefined but may be non-numeric; coerce them
to numbers (e.g., Number(...) or parseInt(..., 10)), validate they are finite
integers (not NaN/Infinity) and throw a clear Error if invalid before
constructing the StringRange and Id; ensure the Id uses the validated numeric
index/length values (and update any uses of index/length in the returned object
to the coerced numeric variables).
- Around line 1-9: The import from 'interlinearizer' is unresolved causing
lint/build errors; either update project module resolution (add tsconfig paths
and configure eslint-import-resolver-typescript) or replace the bare module
import with a correct relative import that points to the module exporting
LexemeData, PunctuationData, ClusterData, StringRange, InterlinearData, and
VerseData used in interlinearXmlParser.ts; locate the import line that currently
reads "import { X } from 'interlinearizer'" and change it to the proper relative
path to the file that defines those exports (or fix tsconfig/eslint resolver) so
the symbols resolve.
…s are properly validated as finite numbers
jasonleenaylor
left a comment
jasonleenaylor
left a comment
There was a problem hiding this comment.
This is a good start, we should probably add unit tests in to verify the parsing behavior on this test data. Start off with good coverage ;)
@jasonleenaylor partially reviewed 8 files and made 3 comments.
Reviewable status: 4 of 10 files reviewed, 3 unresolved discussions (waiting on @alex-rawlings-yyc and @imnasnainaec).
src/types/interlinearizer.d.ts line 34 at r5 (raw file):
Lexemes: LexemeData[]; /** When true, this cluster instance is excluded from the interlinear display. */ Excluded: boolean;
I'm curious about this bit of data. What do you see as the use case for it?
src/types/interlinearizer.d.ts line 70 at r5 (raw file):
BookId: string; /** Verse data keyed by verse reference (e.g. "RUT 3:1"). */ Verses: Record<string, VerseData>;
This makes a concrete claim that there can only be one set of verse data per reference, we would need to have code guarding that in the parsing.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status: 4 of 10 files reviewed, 2 unresolved discussions (waiting on @alex-rawlings-yyc and @jasonleenaylor).
a discussion (no related file):
When I run Platform.Bible with this extension, I get:
Interlinearizer
Raw JSON of the model parsed from test-data/Interlinear_en_MAT.xml.
Parse error
Invalid XML: Range missing required Index or Length attributes
I'm glad for the error handling but suspect the default/demo behavior isn't supposed to trigger it.
package-lock.json line 0 at r5 (raw file):
This changes us from the template's "lockfileVersion": 2, to "lockfileVersion": 3,. I think I support that, but want to make sure it's a conscious choice. And once done, we probably want to avoid anybody taking us back to v2.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
Good call. I've been out of the real world for long enough that I forgot about those entirely. I'll get them done
@alex-rawlings-yyc made 6 comments.
Reviewable status: 4 of 10 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec and @jasonleenaylor).
a discussion (no related file):
Previously, imnasnainaec (D. Ror.) wrote…
When I run Platform.Bible with this extension, I get:
Interlinearizer
Raw JSON of the model parsed from test-data/Interlinear_en_MAT.xml.
Parse error
Invalid XML: Range missing required Index or Length attributesI'm glad for the error handling but suspect the default/demo behavior isn't supposed to trigger it.
That was a rushed change I made. Should've tested it better before pushing. I'll add it to the list
package-lock.json line at r5 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
This changes us from the template's
"lockfileVersion": 2,to"lockfileVersion": 3,. I think I support that, but want to make sure it's a conscious choice. And once done, we probably want to avoid anybody taking us back to v2.
Not a conscious decision. Still not super familiar with npm to I might've done that by accident
src/interlinearizer.web-view.tsx line 2 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
Need to update
'paranext-extension-template'to'interlinearizer'.
Done.
src/main.ts line 15 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
'paranextExtensionTemplate.interlinearizer'->'interlinearizer.<something like mainWebView>'
Done.
src/parsers/interlinearXmlParser.ts line 9 at r3 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
This file has several references to
paranext-extension-templatethat need updating.
Done.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc made 1 comment.
Reviewable status: 4 of 10 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec and @jasonleenaylor).
a discussion (no related file):
Previously, alex-rawlings-yyc wrote…
That was a rushed change I made. Should've tested it better before pushing. I'll add it to the list
Forgot to negate some conditions. Fixed in the latest commit
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 3 of 30 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec and @jasonleenaylor).
- Introduced Jest configuration in `jest.config.ts` and setup file in `jest.setup.ts` for testing. - Added unit tests for the `interlinearizer.web-view` component , `InterlinearXmlParser`, and `main` in `src/__tests__/`. - Created mocks for static assets and dependencies to facilitate testing. - Updated ESLint and Prettier configurations to ignore Jest-related files and directories. - Enhanced README to document the new testing structure and usage. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@jest.config.ts`:
- Around line 65-76: The comment above the coverageThreshold is inconsistent: it
says "Branches at 98% to allow err instanceof Error fallback" while the
coverageThreshold.branches is set to 100; update the comment to reflect 100% (or
change coverageThreshold.branches to 98 if that was intended) by editing the
coverageThreshold block and its preceding comment so they match (refer to the
coverageThreshold object and the branches property in the jest.config.ts
snippet).
🧹 Nitpick comments (6)
__mocks__/web-view-inline.js (1)
5-7: Consider returningnullfor React component semantics.If this mock is ever rendered as a React component, returning
nullis the idiomatic way to render nothing, whereasundefinedwould cause a React warning in strict mode. If this mock is purely for import resolution and never rendered, the current implementation is fine.♻️ Optional fix
function MockWebViewComponent() { - return undefined; + return null; }__mocks__/papi-backend.js (1)
1-33: Potential duplication with__mocks__/@papi/backend.js.This file is nearly identical to
__mocks__/@papi/backend.js. The moduleNameMapper injest.config.tsexplicitly maps@papi/backendto this file, making the scoped directory mock potentially redundant. Consider consolidating to a single mock file and updating either the moduleNameMapper or removing the duplicate to avoid maintenance overhead.__mocks__/@papi/backend.js (1)
1-33: Implementation is correct, but see duplication note above.This is the proper Jest auto-mock location for the scoped
@papi/backendpackage. However, sincejest.config.tsusesmoduleNameMapperto explicitly resolve this module, this file may not be used during test runs. Consider verifying which mock is active and consolidating.src/__tests__/interlinearizer.web-view.test.tsx (1)
34-38: Unconventional but functional component access pattern.Using
require()andglobalThis.webViewComponentworks but couples tests to an internal export mechanism. If the web-view module's export pattern changes, tests will break silently at runtime rather than at compile time.Consider whether the component could be exported directly for testability, or document this pattern if it's a framework constraint.
src/__tests__/main.test.ts (1)
39-44: Simplify the type guard logic.Line 40 has confusing and partially redundant conditions:
x === undefinedis finetypeof x === 'object' && !(x instanceof Object)is convoluted — this attempts to catchnull(which hastypeof === 'object'but isn't an instance ofObject), but the next linetypeof x !== 'object'would returnfalsefornullanyway♻️ Suggested simplification
function isIWebViewProvider(x: unknown): x is IWebViewProvider { - if (x === undefined || (typeof x === 'object' && !(x instanceof Object))) return false; - if (typeof x !== 'object') return false; + if (x == null || typeof x !== 'object') return false; if (!('getWebView' in x)) return false; return typeof x.getWebView === 'function'; }src/parsers/interlinearXmlParser.ts (1)
123-141: Use coerced numeric values in the map step for consistency.The filter at lines 127-129 correctly validates with
Number()andNumber.isFinite(), but the map at lines 135-136 uses the rawrangeElement['@_Index']andrangeElement['@_Length']values instead of the coerced numbers. WhileparseAttributeValue: trueshould ensure these are already numbers, this is inconsistent withextractClustersFromVersewhich explicitly uses the coerced values.♻️ Proposed fix for consistency
return elements .filter((el): el is ParsedPunctuation & { Range: ParsedRange } => { const rangeElement = el.Range; if (!rangeElement) return false; - const indexRaw = Number(rangeElement['@_Index']); - const lengthRaw = Number(rangeElement['@_Length']); - return Number.isFinite(indexRaw) && Number.isFinite(lengthRaw); + const index = Number(rangeElement['@_Index']); + const length = Number(rangeElement['@_Length']); + return Number.isFinite(index) && Number.isFinite(length); }) .map((el) => { const rangeElement = el.Range; return { TextRange: { - Index: rangeElement['@_Index'], - Length: rangeElement['@_Length'], + Index: Number(rangeElement['@_Index']), + Length: Number(rangeElement['@_Length']), }, BeforeText: el.BeforeText ?? '', AfterText: el.AfterText ?? '', }; });
alex-rawlings-yyc
force-pushed
the
xml-parser
branch
from
9b5a8b4 to
8d30282
Compare
alex-rawlings-yyc
force-pushed
the
xml-parser
branch
from
8d30282 to
e7053e8
Compare
…ain, and web-view
|
Re: Re: Rest of nitpicks are valid and taken into account |
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc resolved 1 discussion.
Reviewable status: 3 of 30 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec and @jasonleenaylor).
- Updated README to specify the single mock for `@papi/backend` and its usage in Jest tests. - Modified `web-view-inline.js` mock to return `null` instead of `undefined` for better React compliance. - Removed redundant `@papi/backend` mock file to streamline mock management. - Clarified comments in `interlinearizer.web-view.test.tsx` regarding the web-view module loading pattern. - Improved type handling in `interlinearXmlParser.ts` for index and length attributes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/types/interlinearizer.d.ts`:
- Around line 5-7: Update the JSDoc comment in the types declaration (the top
comment in interlinear types) to reference the correct parser path: replace any
occurrence of "src/interlinear/interlinearXmlParser.ts" with
"src/parsers/interlinearXmlParser.ts" so the documentation points to the actual
file; locate the comment near the Interlinear types (InterlinearData, VerseData,
ClusterData) declaration to make the edit.
🧹 Nitpick comments (2)
src/__tests__/main.test.ts (2)
25-34: Type guard has redundant checks.The
m !== undefinedcheck on line 28 is redundant sincetypeof m === 'object'already handlesundefined(which has type'undefined'). Similarly,m instanceof Objecton line 29 is unnecessary after confirmingtypeof m === 'object'andm !== null(implicit via checking properties exist).♻️ Simplified type guard
function isPapiBackendTestMock(m: unknown): m is PapiBackendTestMock { return ( typeof m === 'object' && - m !== undefined && - m instanceof Object && + m !== null && '__mockRegisterWebViewProvider' in m && '__mockOpenWebView' in m && '__mockLogger' in m ); }
40-45: Simplify the type guard logic.The current conditions on lines 41-42 are convoluted. The intent is clearer with a straightforward null/object check.
♻️ Cleaner type guard
function isIWebViewProvider(x: unknown): x is IWebViewProvider { - if (x === undefined || (typeof x === 'object' && !(x instanceof Object))) return false; - if (typeof x !== 'object') return false; - if (!('getWebView' in x)) return false; - return typeof x.getWebView === 'function'; + return ( + typeof x === 'object' && + x !== null && + 'getWebView' in x && + typeof x.getWebView === 'function' + ); }
- Correct path for `interlinearXmlParser.ts` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Looking good! Just a handful of small questions and comments.
Also, let's add a .github/workflows/test.yml.
@imnasnainaec reviewed 6 files and all commit messages, and made 7 comments.
Reviewable status: 8 of 30 files reviewed, 3 unresolved discussions (waiting on @alex-rawlings-yyc and @jasonleenaylor).
package-lock.json line at r5 (raw file):
Previously, alex-rawlings-yyc wrote…
Not a conscious decision. Still not super familiar with npm to I might've done that by accident
To clarify, I don't expect it was a conscious decision when it happened. I just hope it is a conscious decision to merge in the lockfileVersion update from 2 to 3. Two things I'm considering:
- We wouldn't want it unintentionally reverted to 2 by another dev setup, but if all devs are on newer version of npm, we probably don't need to explicitly enforce that its stays at 3.
- Whenever we sync with the template, we should roughly track with their
package.json, but we can probably just ignore theirpackage-lock.json, regenerating it ourselves.
package.json line 30 at r10 (raw file):
"lint-fix:scripts": "npm run format && npm run lint:scripts", "bump-versions": "ts-node ./lib/bump-versions.ts", "test": "jest",
A warning when I run npm run test:
ts-jest[ts-compiler] (WARN) Got a `.js` file to compile while `allowJs` option is not set to `true` (file: ~\paranext\interlinearizer-extension\__mocks__\platform-bible-utils.js). To fix this:
- if you want TypeScript to process JS files, set `allowJs` to `true` in your TypeScript config (usually tsconfig.json)
- if you do not want TypeScript to process your `.js` files, in your Jest config change the `transform` key which value is `ts-jest` so that it does not match `.js` files anymore
src/__tests__/parsers/interlinearXmlParser.test.ts line 8 at r10 (raw file):
* defaults, VerseData.Cluster ?? [], Verses.item ?? []), and all documented parse errors (missing * root, GlossLanguage/BookId, Verses, Cluster Range, Range Index/Length, Lexeme Id). Maintains 100% * statement, branch, function, and line coverage for the parser.
⛏️ I don't think we need a mention of the test coverage here in an individual test file, since that's enforced globally in jest.config.ts.
src/__tests__/parsers/interlinearXmlParser.test.ts line 15 at r10 (raw file):
import * as path from 'path'; import { InterlinearXmlParser } from '../../parsers/interlinearXmlParser';
⛏️ ❓ Since all the test files are in src/__tests__/ and not alongside the files they are testing, it might be nice to default their import statements to be relative to src/ (e.g., 'parsers/interlinearXmlParser') rather than having to (../)+ all the way back to src/.
src/__tests__/parsers/interlinearXmlParser.test.ts line 110 at r10 (raw file):
{ LexemeId: 'Suffix:ing', SenseId: 'g2' }, ]); expect(cluster.LexemesId).toBe('Stem:hello/Suffix:ing');
❓ Since LexemesId and Id are /-separated, how does the parser handle it if an XML string contains a /? Or would that be invalid? Whatever the case, a test for that situation may be warranted.
jest.config.ts line 35 at r10 (raw file):
'src/parsers/**/*.ts', 'src/main.ts', 'src/interlinearizer.web-view.tsx',
⛏️ 'src/interlinearizer.web-view.tsx' can probably be generalized to 'src/**/*.web-view.tsx' to cover future WebViews. (And the docstring above updated to match.)
|
Previously, imnasnainaec (D. Ror.) wrote…
TODO I'll have a look at Paratext 9 to see whether |
|
Previously, imnasnainaec (D. Ror.) wrote…
By "regenerating it ourselves", do you mean regenerating it as individuals and leaving the |
|
Previously, imnasnainaec (D. Ror.) wrote…
I've converted the mocks to |
|
Previously, alex-rawlings-yyc wrote…
The ID concatenation system is internal to Paratext 9, so we can revisit better ways to store these IDs in our model later. For now, I think we can ignore this issue since the XML files don’t store the |
…ionality - Added project path to TypeScript resolver in `.eslintrc.js`. - Updated Jest configuration in `jest.config.ts` to simplify module resolution for imports. - Removed outdated mock files and updated existing mocks to TypeScript. - Refactored import paths in test files to utilize new module resolution strategy. - Enhanced README to clarify GitHub Actions workflows and testing structure. - Updated `tsconfig.json` to include paths for src-rooted imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@__mocks__/platform-bible-utils.ts`:
- Around line 51-56: The current runAllUnsubscribers() treats undefined (void)
as failure because it returns results.every(Boolean); update the final check to
treat only an explicit false as failure, e.g. replace results.every(Boolean)
with results.every(r => r !== false) in the runAllUnsubscribers function so
void-returning unsubscribers are considered success while explicit false still
signals failure.
🧹 Nitpick comments (5)
tsconfig.json (1)
59-63: The"main"path alias is ambiguous and may conflict with theno-restricted-importsrule.The alias name
"main"is generic and could be confused with thepackage.json"main"field or Node's built-in resolution. Additionally,.eslintrc.jsline 69 restricts imports matching the patternmain/*, which could interfere if the alias is ever extended tomain/*. Consider a more specific alias like"@main"or"src/main"to avoid ambiguity..github/workflows/test.yml (2)
24-28: Pin theparanext-corecheckout to a specific ref or tag.Checking out
paranext-corewithout arefdefaults to its default branch. If upstream makes breaking changes, your CI will fail unpredictably. Consider pinning to a known-compatible tag or SHA to make builds reproducible.Proposed fix
- name: Checkout paranext-core repo to use its sub-packages uses: actions/checkout@v4 with: path: paranext-core repository: paranext/paranext-core + ref: 'v1.0.0' # Pin to a compatible release tag
14-17: Single-entry matrix is unnecessary.The
strategy.matrixwith only[ubuntu-latest]adds indirection with no benefit. You can simplify by usingruns-on: ubuntu-latestdirectly, or add more OSes if cross-platform testing is intended.src/parsers/interlinearXmlParser.ts (2)
133-155: Minor:Number()is called twice on the same attributes — once in filter, once in map.The filter (lines 140-141) already coerces and validates the values as finite numbers. The map (lines 148-149) repeats the
Number()coercion. You could store the validated values in the filter phase (e.g., using a different pattern) or accept the minor redundancy. Not a correctness issue.
224-233: Remove redundant'item'entry; consider fully-qualified paths for better precision.The standalone
'item'entry is redundant with the more specific'InterlinearData.Verses.item'sinceendsWith()matching on the full path already covers it. While the broad matching withendsWith('Cluster'),endsWith('Lexeme'), etc. currently works correctly for the fixed schema, using fully-qualified paths would be more defensive against accidental schema changes.♻️ Suggested tightening
const arrayPaths = [ 'InterlinearData.Verses.item', - 'item', - 'Cluster', - 'Punctuation', - 'Lexeme', + 'InterlinearData.Verses.item.VerseData.Cluster', + 'InterlinearData.Verses.item.VerseData.Punctuation', + 'InterlinearData.Verses.item.VerseData.Cluster.Lexeme', ]; - return arrayPaths.some((path) => jPath.endsWith(path)); + return arrayPaths.includes(jPath);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 103: Update the README entry that currently mentions the mock filename
`__mocks__/papi-backend.js` to the correct TypeScript filename
`__mocks__/papi-backend.ts` so the documented Jest mock name matches the actual
file; search for the exact string `__mocks__/papi-backend.js` and replace it
with `__mocks__/papi-backend.ts`.
🧹 Nitpick comments (1)
README.md (1)
93-94: Clarify the “required” status of package-lock.json.Saying package-lock.json is “required for Platform.Bible to use the extension” is likely inaccurate; it’s primarily for dependency reproducibility. Consider rephrasing to avoid implying runtime requirement.
- Updated ESLint configuration to use an absolute path for TypeScript resolver in `.eslintrc.js`. - Modified Jest configuration to align path aliases with TypeScript settings in `tsconfig.json`. - Enhanced README to provide clearer descriptions of unit tests and their structure. - Improved test imports to utilize new path aliases for better readability. - Added new test cases in `interlinearXmlParser.test.ts` to validate punctuation handling.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
1 questions and 3 nitpicks. (You're always welcome to ignore the ⛏️s from me.) There are also 2 unanswered comments from Jason.
I've finished reviewing the WebView stuff and plan to look over the parser part tomorrow (unless somebody else approves before I get to it).
@imnasnainaec partially reviewed 20 files and all commit messages, made 6 comments, and resolved 2 discussions.
Reviewable status: 27 of 33 files reviewed, 1 unresolved discussion (waiting on @alex-rawlings-yyc and @jasonleenaylor).
package-lock.json line at r5 (raw file):
Previously, alex-rawlings-yyc wrote…
By "regenerating it ourselves", do you mean regenerating it as individuals and leaving the
package-lock.jsonin the repo iun sync with the template?
I mean letting the package-lock.json be out of sync with the template, so that whoever synces with an updated template will have to also regenerate our package-lock.json if they have bumped any versions in package.json.
jest.config.ts line 126 at r13 (raw file):
'^@papi/backend$': '<rootDir>/__mocks__/papi-backend.ts', /** Mock Platform core types; main.ts imports types only (erased at runtime). */ '^@papi/core$': '<rootDir>/__mocks__/papi-core.ts',
⛏️ We're not using them yet, but we probably want to also have mocks for @papi/frontend and @papi/frontend/react (mentioned in webpack\webpack.config.base.ts). We may well use both, since we are using both in the FW Lite extension.
src/__tests__/interlinearizer.web-view.test.tsx line 21 at r13 (raw file):
/** * Load the web-view module; it assigns the component to globalThis.webViewComponent. This pattern
⛏️ In text/comments, web-view -> WebView to align with the PB convention.
src/__tests__/interlinearizer.web-view.test.tsx line 87 at r13 (raw file):
render(<InterlinearizerWebView {...testWebViewProps} />); expect(screen.queryByText(/^parse error$/i)).not.toBeInTheDocument();
⛏️ Mismatched regex expectation: This test is for no /^parse error$/i, but the parse error test below only has /parse error/i without ^ or $.
src/__tests__/main.test.ts line 42 at r13 (raw file):
} jest.mock('@papi/backend', () => {
❓ Is this mock not redundant with __mocks__/papi-backend.ts?
- Updated Jest configuration in `jest.config.ts` to include new mocks for `@papi/frontend` and `@papi/frontend/react`. - Improved comments in `jest.config.ts` for better understanding of mock purposes. - Clarified README to specify the usage of `@papi/backend` and `@papi/frontend` mocks in tests. - Refined comments in `interlinearizer.web-view.tsx` and `interlinearizer.web-view.test.tsx` for better readability and understanding of the code structure.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/__tests__/parsers/interlinearXmlParser.test.ts`:
- Around line 130-156: The test title "parses multiple lexemes when one Id
contains slash: join produces unambiguous LexemesId" is misleading because the
joined LexemesId can be ambiguous; update the test title to accurately reflect
behavior (e.g., "join preserves slash in LexemesId" or "preserves slash when
joining Lexeme Ids") so it no longer asserts unambiguous semantics; keep the
test body and assertions using parser.parse and expectations on cluster.Lexemes,
cluster.LexemesId, and cluster.Id unchanged.
In `@src/interlinearizer.web-view.tsx`:
- Around line 23-25: The catch block that returns { data: undefined, error: new
Error(String(err)).message } double-wraps Error objects; change it to return the
raw message by using a conditional that extracts err.message when err instanceof
Error, otherwise use String(err) (i.e., replace new Error(String(err)).message
with err instanceof Error ? err.message : String(err)) so the returned error
string is not prefixed with "Error: ". Keep the return shape ({ data: undefined,
error: ... }) exactly the same.
🧹 Nitpick comments (2)
jest.config.ts (1)
12-246: Consider removing the commented-out boilerplate.The file retains ~100 lines of commented-out default Jest config options (from
jest --init). They add noise without value since the Jest docs link is already at the top. Removing them would make the actual configuration much easier to scan.__mocks__/papi-frontend-react.ts (1)
14-33: Sharedjest.fn()indefaultDataHookReturnmay leak state between tests.The
defaultDataHookReturntuple (and itsjest.fn()setter at index 1) is a single shared instance returned by every Proxy-backed hook. If a test's component calls the setter, those call records persist into subsequent tests unlessjest.clearAllMocks()/jest.resetAllMocks()runs between tests. Consider creating the tuple inside the factory so each hook invocation gets a fresh mock, or document that abeforeEach(jest.clearAllMocks)is expected.♻️ Option: fresh tuple per invocation
const createUseDataLikeHook = () => jest.fn(() => new Proxy( {}, { - get: () => () => defaultDataHookReturn, + get: () => () => [undefined, jest.fn(), false], }, ), );
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
I think I've reviewed everyone's comments now. I'm still not quite used to Reviewable so there's a chance I haven't addressed everything yet. Let me know if I've missed anything. Nitpicks are always appreciated as I get back into the groove of things
@alex-rawlings-yyc partially reviewed 6 files, made 4 comments, and resolved 1 discussion.
Reviewable status: 26 of 35 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec).
src/__tests__/main.test.ts line 42 at r13 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
❓ Is this mock not redundant with
__mocks__/papi-backend.ts?
Sure is; thanks for catching
src/types/interlinearizer.d.ts line 34 at r5 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
I'm curious about this bit of data. What do you see as the use case for it?
Excluded was a holdover from PT9 and I've removed it.
src/types/interlinearizer.d.ts line 70 at r5 (raw file):
Previously, jasonleenaylor (Jason Naylor) wrote…
This makes a concrete claim that there can only be one set of verse data per reference, we would need to have code guarding that in the parsing.
Code has been added to guard it
…mponent - Updated `jest.config.ts` to improve comments and clarify configuration options. - Modified error handling in `interlinearizer.web-view.tsx` to ensure consistent error message formatting for both Error and non-Error types. - Enhanced tests in `interlinearizer.web-view.test.tsx` to cover new error handling scenarios, ensuring robust error display for various input types. - Updated test case descriptions for clarity in `interlinearXmlParser.test.ts`.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc partially reviewed 3 files and resolved 1 discussion.
Reviewable status: 29 of 35 files reviewed, all discussions resolved (waiting on @imnasnainaec).
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec partially reviewed 8 files and all commit messages, and made 2 comments.
Reviewable status: 32 of 35 files reviewed, 1 unresolved discussion (waiting on @alex-rawlings-yyc).
src/parsers/interlinearXmlParser.ts line 197 at r15 (raw file):
* Input is a raw XML string (caller is responsible for obtaining it, e.g. from file or network). * Output matches the types in `interlinearizer`; no extra conversion is done. Expects the * interlinear XML schema described in the project README.
❓ This docstring mentions an "interlinear XML schema", but I don't see such a thing in the README (and "schema" has no other xml-related hits in this branch. Would it be worth creating a dedicated XML schema file?
__mocks__/papi-frontend-react.ts line 10 at r15 (raw file):
* useScrollGroupScrRef, useSetting, useProjectData, useProjectDataProvider, useProjectSetting, * useDialogCallback, useDataProviderMulti, useLocalizedStrings, useWebViewController, * useRecentScriptureRefs. Each hook is a jest.fn() with a sensible default return so components
⛏️ Listing all the hooks here in the comment adds a maintenance burden.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec reviewed 3 files and made 1 comment.
Reviewable status: 34 of 35 files reviewed, 1 unresolved discussion (waiting on @alex-rawlings-yyc).
src/parsers/interlinearXmlParser.ts line 197 at r15 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
❓ This docstring mentions an "interlinear XML schema", but I don't see such a thing in the README (and "schema" has no other xml-related hits in this branch. Would it be worth creating a dedicated XML schema file?
I talked with Jason about schemas. We don't need to create one for the task of bringing in data from PT9. We should just reconcile this comment and the README.
- Introduced a comprehensive section on the interlinear XML schema, detailing the expected document structure, attributes, and child elements. - Provided examples of minimal and full valid XML documents to illustrate the schema usage. - Enhanced clarity on the parser's output structure and types defined in `src/types/interlinearizer.d.ts`.
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
@alex-rawlings-yyc reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @alex-rawlings-yyc).
src/parsers/interlinearXmlParser.ts line 197 at r15 (raw file):
Previously, imnasnainaec (D. Ror.) wrote…
I talked with Jason about schemas. We don't need to create one for the task of bringing in data from PT9. We should just reconcile this comment and the README.
Added a section about that in the README. I think we should eventually have a document specifically covering the data models once we've nailed down what the new one will be, but for now the PT9 schema breakdown can probably live there. Or we can remove it from the README and delete this comment. Either way works for me.
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
Approved with 2 more optional ⛏️s.
@imnasnainaec partially reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status:
src/__tests__/parsers/interlinearXmlParser.test.ts line 287 at r16 (raw file):
<Range Index="5" Length="1" /> <BeforeText>valid</BeforeText> <AfterText></AfterText>
⛏️ Since <AfterText> is optional (per the schema in the README), you might as well drop (some or all of) the empty <AfterText></AfterText> occurrences in this test.
__mocks__/papi-backend.ts line 4 at r16 (raw file):
* Jest mock for @papi/backend. Provides papi and logger so main.ts can be unit-tested without * loading the real Platform API. */
⛏️ I just learned that the file-level JSDoc docstring (/** [...] */) at the top of a JS/TS file should contain @file to be associated with the file. This applies to most of the files in this pr, but for this file in particular:
/**
* Jest mock for @papi/backend. Provides papi and logger so main.ts can be unit-tested without
* loading the real Platform API.
*/
should probably be
/**
* @file Jest mock for `@papi/backend`. Provides `papi` and `logger` so `main.ts` can be unit-tested
* without loading the real Platform API.
*/
alex-rawlings-yyc
left a comment
alex-rawlings-yyc
left a comment
There was a problem hiding this comment.
Valid nitpicks. Both addressed.
@alex-rawlings-yyc partially reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:
- Added @file annotations to the comments in various Jest mock files to standardize documentation and improve clarity on their purpose. - Updated unit test files to reflect the same @file annotation format for consistency in documentation style. Co-authored-by: Cursor <[email protected]>
alex-rawlings-yyc
force-pushed
the
xml-parser
branch
from
845dd46 to
2d38026
Compare
imnasnainaec
left a comment
imnasnainaec
left a comment
There was a problem hiding this comment.
@imnasnainaec partially reviewed 17 files and all commit messages.
Reviewable status:
3 participants
interlinearizer.web-view.tsxandinterlinearizer.web-view.scssfor rendering interlinear data.InterlinearXmlParserfor parsing interlinear XML data.package.jsonandpackage-lock.jsonto includefast-xml-parserdependency.interlinearizer.d.ts.test-data/Interlinear_en_MAT.xml.main.tsto register the new web view provider for the interlinearizer.This change is
Summary by CodeRabbit
New Features
Chores
Documentation
Style