Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
tests/powerpoint.spec.ts (2)
156-167: ⚡ Quick winAvoid order-coupled assertions in bulk update verification.
The test currently assumes deterministic shape ordering after update. Matching by semantic identity (e.g., body) is safer and avoids flaky failures from internal reorder behavior.
Suggested change
- expectTypstShape(shapes[0], { + const shapeA = shapes.find((shape) => shape.customXml[0]?.includes("<pptypst:body>a</pptypst:body>")); + const shapeB = shapes.find((shape) => shape.customXml[0]?.includes("<pptypst:body>b</pptypst:body>")); + expect(shapeA).toBeDefined(); + expect(shapeB).toBeDefined(); + + expectTypstShape(shapeA as ShapeSnapshot, { body: "a", preamble: "`#let` a = 1", fontSize: "42", fillColor: "`#112233`", }); - expectTypstShape(shapes[1], { + expectTypstShape(shapeB as ShapeSnapshot, { body: "b", preamble: "`#let` b = 2", fontSize: "42", fillColor: "disabled", });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/powerpoint.spec.ts` around lines 156 - 167, The assertions are order-dependent: instead of assuming shapes[0]/shapes[1] correspond to bodies "a" and "b", locate shapes by their semantic identity (e.g., find the shape whose body === "a" and the one whose body === "b") and call expectTypstShape on those found objects; update the test to search the shapes array (using Array.prototype.find or similar) for body "a" and "b" and then assert their preamble, fontSize, and fillColor so the test no longer relies on array ordering.
47-53: ⚡ Quick winPrefer XML parsing over raw substring checks for preamble/body assertions.
These assertions are brittle because
XMLSerializerescapes text content; valid Typst content containing XML-sensitive characters can fail this test despite correct behavior.Suggested change
function expectTypstShape(shape: ShapeSnapshot, expected: { body: string; preamble: string; fontSize: string; fillColor: string; }) { @@ expect(shape.customXml).toHaveLength(1); - if (expected.preamble) { - expect(shape.customXml[0]).toContain(`<pptypst:preamble>${expected.preamble}</pptypst:preamble>`); - } else { - expect(shape.customXml[0]).toMatch(/<pptypst:preamble\s*\/>|<pptypst:preamble><\/pptypst:preamble>/); - } - expect(shape.customXml[0]).toContain(`<pptypst:body>${expected.body}</pptypst:body>`); + const xml = new DOMParser().parseFromString(shape.customXml[0], "application/xml"); + const preamble = xml.getElementsByTagNameNS("*", "preamble")[0]?.textContent ?? ""; + const body = xml.getElementsByTagNameNS("*", "body")[0]?.textContent ?? ""; + expect(preamble).toBe(expected.preamble); + expect(body).toBe(expected.body); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/powerpoint.spec.ts` around lines 47 - 53, Replace brittle substring checks on shape.customXml[0] with proper XML parsing: parse shape.customXml[0] into an XML/DOM document, query for the pptypst:preamble and pptypst:body elements (e.g., using DOMParser or an XML parser used in tests), then assert element existence and compare their textContent to expected.preamble and expected.body (handling the case when expected.preamble is absent by asserting the preamble element is empty or self-closing). Update the assertions that reference shape.customXml[0], expected.preamble, and expected.body in the test to use the parsed element text instead of raw string containment/regex.tests/_support/office-mock.ts (1)
21-32: ⚡ Quick winCache compiled Office mock payload once per worker.
Line 21 recompiles/transpiles the same sources for each routed request. Memoizing this result removes repeated fs + transpile overhead and speeds e2e runs.
Suggested refactor
+let compiledOfficeMockPromise: Promise<string> | null = null; + async function compileOfficeMock() { + if (compiledOfficeMockPromise) { + return compiledOfficeMockPromise; + } + + compiledOfficeMockPromise = (async () => { const { compileBrowserMockSource } = await import("./transpile-browser-mock"); const sourceParts = await Promise.all( officeMockFiles.map(fileName => fs.readFile(path.join(officeMockDir, fileName), "utf8")), ); return compileBrowserMockSource( officeMockDir, sourceParts.join("\n\n"), path.join(officeMockDir, "index.ts"), ); + })(); + + return compiledOfficeMockPromise; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_support/office-mock.ts` around lines 21 - 32, The compileOfficeMock function currently reads and transpiles office mock files on every call causing repeated fs and transpile overhead; add a module-level cache (store the compiled payload or preferably the Promise) keyed per worker and have compileOfficeMock return the cached value when present instead of re-reading; keep existing behavior and use the same symbols (compileOfficeMock, compileBrowserMockSource, officeMockFiles, officeMockDir) so callers are unchanged, and ensure the cache is set before/after awaiting the transpile Promise to avoid duplicate concurrent work.tests/_support/office/document-model.ts (1)
94-104: ⚡ Quick winConsider validating DOMParser results for clearer test failures.
DOMParser.parseFromString()doesn't throw on malformed XML; it creates a document with aparsererrorelement. If test code passes invalid XML, accessing.namespaceURImay return unexpected values, making failures harder to diagnose.🛡️ Proposed validation
private addCustomXmlPart(xml: string): MockXmlPart { const documentNode = new DOMParser().parseFromString(xml, "application/xml"); + if (documentNode.documentElement.tagName === "parsererror") { + throw new Error(`Invalid XML in custom XML part: ${xml}`); + } const part = new MockXmlPart( this.documentModel.nextXmlPartId(), xml, documentNode.documentElement.namespaceURI, ); this.xmlParts.push(part); return part; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_support/office/document-model.ts` around lines 94 - 104, In addCustomXmlPart, validate the DOMParser result before using documentNode.documentElement.namespaceURI: after calling DOMParser.parseFromString(xml, "application/xml") check that documentNode.documentElement exists and that documentNode.getElementsByTagName('parsererror') (or equivalent) contains no entries; if a parse error is found throw or assert with a clear message including the original xml so tests fail with a readable error; update references to MockXmlPart(...) to only run when the parser result is valid.tests/_support/browser-mocks/typst.ts (1)
37-39: 💤 Low valueSame async error handling pattern as createTypstCompiler.
While the renderer mock doesn't currently throw, if validation or error handling is added later, ensure errors in
initare returned as rejected promises rather than thrown synchronously, for consistency with the async method signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_support/browser-mocks/typst.ts` around lines 37 - 39, The init method on the renderer mock should propagate errors as rejected promises instead of throwing synchronously; update init (the function pushing to typstMockState.rendererInitOptions) to wrap its logic in a try/catch and return Promise.resolve() on success or Promise.reject(err) in the catch (mirror the async error handling used in createTypstCompiler) so any validation/error inside init is returned as a rejected promise rather than thrown.tests/_support/transpile-browser-mock.ts (1)
25-28: 💤 Low valueConsider adding error handling for better test diagnostics.
If
fs.readFilefails (e.g., file not found) orts.transpileModuleencounters syntax errors, the error will propagate but may lack context about which mock file failed. Adding try-catch with contextual error messages would improve debugging:export async function compileBrowserMock(filePath: string) { try { const source = await fs.readFile(filePath, "utf8"); return compileBrowserMockSource(filePath, source, filePath); } catch (error) { throw new Error(`Failed to compile browser mock at ${filePath}: ${error}`); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_support/transpile-browser-mock.ts` around lines 25 - 28, The compileBrowserMock function currently awaits fs.readFile and calls compileBrowserMockSource without contextual error handling; wrap the body of compileBrowserMock in a try-catch that catches any error from fs.readFile or downstream steps (including ts.transpileModule inside compileBrowserMockSource), and rethrow a new Error that includes the failing filePath and the original error message (e.g., `Failed to compile browser mock at ${filePath}: ${error}`) so test failures report which mock file failed while preserving the original error details.tests/_support/typst-mock.ts (1)
41-47: 💤 Low valueConsider documenting waitForFunction timeout behavior.
The
waitUntilReady()method relies on Playwright's defaultwaitForFunctiontimeout (usually 30s from config). If the mocked renderer never callsinit, the test will hang until timeout.While the default timeout is likely sufficient, adding a brief comment or explicit timeout option would clarify the expected behavior:
/** Waits for the mocked renderer init call, which means the Typst wrapper initialized. * Throws TimeoutError if renderer does not initialize within the default timeout. */ async waitUntilReady() { await this.page.waitForFunction(async (moduleUrl) => { ... }, stateModuleUrl, { timeout: 30_000 }); // explicit 30s timeout }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/_support/typst-mock.ts` around lines 41 - 47, The waitUntilReady method uses Playwright's waitForFunction and relies on the default timeout, which can cause tests to hang if the mocked renderer never calls init; update the waitUntilReady implementation to (1) add a short JSDoc comment explaining expected behavior and that it will throw a TimeoutError if the renderer doesn't initialize, and (2) pass an explicit timeout option to page.waitForFunction (e.g., { timeout: 30_000 }) when calling it with stateModuleUrl so typstMockReady() failing to be true fails fast; update the function named waitUntilReady and the waitForFunction call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Line 15: Replace the floating GitHub Actions tags with immutable commit SHAs
for each `uses:` entry to prevent silent retagging: update
`actions/checkout@v6`, `actions/setup-node@v6`, and `actions/upload-artifact@v7`
to their corresponding full commit SHA pins (replace the tag with the
repository@<full-commit-sha> values), ensuring each `uses:` line references the
exact commit; verify the SHAs are correct by copying them from the official
action repository releases/tags before committing.
- Around line 14-16: The checkout step uses actions/checkout@v6 and currently
leaves credential persistence enabled; update the checkout job step to include a
with block setting persist-credentials: false (i.e., add "with:
persist-credentials: false" under the actions/checkout@v6 step) so the auth
token is not persisted to the local git config for subsequent steps.
In `@tests/_support/browser-mocks/typst.ts`:
- Around line 19-23: The init(options: CompilerInitOptions) method currently
throws synchronously on invalid getModule which breaks async callers; change it
to consistently return a rejected promise (or make the function async) so errors
are delivered as promise rejections—e.g., inside init(options:
CompilerInitOptions) return Promise.reject(new Error("Expected Typst compiler
getModule option.")) instead of throwing, or declare the function async and keep
the throw so callers receive a rejected promise; update the init symbol
accordingly.
In `@tests/pages/powerpoint-page.ts`:
- Around line 139-143: The clearSelection method currently uses a hardcoded
default "slide-1" which can target the wrong slide; change the signature of
clearSelection to accept an optional slideId (remove the "= \"slide-1\""
default) so it defaults to undefined, and ensure the call to this.page.evaluate
passes the slideId (which will be undefined when not provided) so the mock API's
optional slide id behavior is preserved (refer to the clearSelection method in
powerpoint-page.ts).
---
Nitpick comments:
In `@tests/_support/browser-mocks/typst.ts`:
- Around line 37-39: The init method on the renderer mock should propagate
errors as rejected promises instead of throwing synchronously; update init (the
function pushing to typstMockState.rendererInitOptions) to wrap its logic in a
try/catch and return Promise.resolve() on success or Promise.reject(err) in the
catch (mirror the async error handling used in createTypstCompiler) so any
validation/error inside init is returned as a rejected promise rather than
thrown.
In `@tests/_support/office-mock.ts`:
- Around line 21-32: The compileOfficeMock function currently reads and
transpiles office mock files on every call causing repeated fs and transpile
overhead; add a module-level cache (store the compiled payload or preferably the
Promise) keyed per worker and have compileOfficeMock return the cached value
when present instead of re-reading; keep existing behavior and use the same
symbols (compileOfficeMock, compileBrowserMockSource, officeMockFiles,
officeMockDir) so callers are unchanged, and ensure the cache is set
before/after awaiting the transpile Promise to avoid duplicate concurrent work.
In `@tests/_support/office/document-model.ts`:
- Around line 94-104: In addCustomXmlPart, validate the DOMParser result before
using documentNode.documentElement.namespaceURI: after calling
DOMParser.parseFromString(xml, "application/xml") check that
documentNode.documentElement exists and that
documentNode.getElementsByTagName('parsererror') (or equivalent) contains no
entries; if a parse error is found throw or assert with a clear message
including the original xml so tests fail with a readable error; update
references to MockXmlPart(...) to only run when the parser result is valid.
In `@tests/_support/transpile-browser-mock.ts`:
- Around line 25-28: The compileBrowserMock function currently awaits
fs.readFile and calls compileBrowserMockSource without contextual error
handling; wrap the body of compileBrowserMock in a try-catch that catches any
error from fs.readFile or downstream steps (including ts.transpileModule inside
compileBrowserMockSource), and rethrow a new Error that includes the failing
filePath and the original error message (e.g., `Failed to compile browser mock
at ${filePath}: ${error}`) so test failures report which mock file failed while
preserving the original error details.
In `@tests/_support/typst-mock.ts`:
- Around line 41-47: The waitUntilReady method uses Playwright's waitForFunction
and relies on the default timeout, which can cause tests to hang if the mocked
renderer never calls init; update the waitUntilReady implementation to (1) add a
short JSDoc comment explaining expected behavior and that it will throw a
TimeoutError if the renderer doesn't initialize, and (2) pass an explicit
timeout option to page.waitForFunction (e.g., { timeout: 30_000 }) when calling
it with stateModuleUrl so typstMockReady() failing to be true fails fast; update
the function named waitUntilReady and the waitForFunction call accordingly.
In `@tests/powerpoint.spec.ts`:
- Around line 156-167: The assertions are order-dependent: instead of assuming
shapes[0]/shapes[1] correspond to bodies "a" and "b", locate shapes by their
semantic identity (e.g., find the shape whose body === "a" and the one whose
body === "b") and call expectTypstShape on those found objects; update the test
to search the shapes array (using Array.prototype.find or similar) for body "a"
and "b" and then assert their preamble, fontSize, and fillColor so the test no
longer relies on array ordering.
- Around line 47-53: Replace brittle substring checks on shape.customXml[0] with
proper XML parsing: parse shape.customXml[0] into an XML/DOM document, query for
the pptypst:preamble and pptypst:body elements (e.g., using DOMParser or an XML
parser used in tests), then assert element existence and compare their
textContent to expected.preamble and expected.body (handling the case when
expected.preamble is absent by asserting the preamble element is empty or
self-closing). Update the assertions that reference shape.customXml[0],
expected.preamble, and expected.body in the test to use the parsed element text
instead of raw string containment/regex.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baeccf6d-abe5-4690-9b4b-00b877d30b19
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
.github/workflows/test.yml.gitignore.vscode/extensions.jsonDEV.mdpackage.jsonplaywright.config.tstests/_support/browser-mocks/typst-memory-access-model.tstests/_support/browser-mocks/typst-options.tstests/_support/browser-mocks/typst-package-registry.tstests/_support/browser-mocks/typst-state-module.d.tstests/_support/browser-mocks/typst-state.tstests/_support/browser-mocks/typst-wasm-url.tstests/_support/browser-mocks/typst.tstests/_support/fixtures.tstests/_support/office-mock.tstests/_support/office/adapter.tstests/_support/office/document-model.tstests/_support/office/install.tstests/_support/office/office-primitives.tstests/_support/office/types.tstests/_support/transpile-browser-mock.tstests/_support/typst-mock.tstests/pages/powerpoint-page.tstests/powerpoint.spec.tstests/preview.spec.tstsconfig.json
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
1 participant
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.
Fixes #36 by initializing the setup for Playwright tests and adds tests that cover the most important functionality of the app.
The code changes were generated in close collaboration with AI, especially since otherwise it would have taken me way too long to mock the PowerPoint API correctly. I did several refactor passes, yet the
tests/_support/office/folder probably has still some room for improvement in terms of code quality. But the tests work now and test exactly what I wanted to test, so I'm happy with this result and will rather spend my energy improving the overall product. Cheers.