Add interlinearizer feature with XML parsing and web view integration#6
Add interlinearizer feature with XML parsing and web view integration#6alex-rawlings-yyc wants to merge 14 commits intomainfrom
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.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new InterlinearXmlParser and TypeScript types, a React WebView component and styles, comprehensive Jest test infrastructure with mocks and test data, updates extension activation/provider flows, CI test workflow, and package/test tooling (including fast-xml-parser dependency and test scripts). Changes
Sequence Diagram(s)sequenceDiagram
participant Ext as Extension (main.ts)
participant API as `@papi/backend`
participant Provider as WebViewProvider
participant WebView as WebView System
Note over Ext,API: Activation Flow
Ext->>API: activate(context)
Ext->>Provider: construct provider with getWebView()
Ext->>API: registerWebViewProvider(mainWebViewType, Provider)
API-->>Ext: registration disposable
Ext->>Ext: store disposable in context
Ext->>API: openWebView(mainWebViewType)
API->>Provider: getWebView(mainWebViewType)
Provider->>WebView: return WebViewDefinition (title, content, styles)
WebView->>WebView: initialize/render content
Ext->>API: logger.info('finished activating')
Note over Ext,API: Deactivation Flow
Ext->>Ext: deactivate()
Ext->>API: logger.info('deactivating...')
Ext->>Ext: run unsubscribers / dispose
Ext-->>API: return boolean
sequenceDiagram
participant Component as InterlinearWebView
participant Parser as InterlinearXmlParser
participant Renderer as ReactRenderer
Note over Component,Parser: Parsing & Render
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 Suggested labels
Suggested reviewers
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 <cursoragent@cursor.com>
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.
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
Chores