paragraph counts. restore unwrapped and other fixes#10
paragraph counts. restore unwrapped and other fixes#10
Conversation
Summary of ChangesHello @lgandecki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the robustness and data quality of chapter processing within the pipeline, alongside enhancing the user experience for character representation. Key changes include the introduction of accurate paragraph counting for better progress tracking, a mechanism to restore unwrapped HTML blocks from LLM outputs, and a suite of tools to normalize legacy play chapter formats. Additionally, the system now intelligently handles unknown characters by generating generic avatars and descriptive slugs, and the LLM capabilities have been expanded with Azure AI SDK integration. These updates collectively aim to provide more reliable content processing and a richer, more consistent experience for users interacting with book content and characters. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Preview ready: No book assets changed in this run. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant number of improvements and fixes to the content processing pipeline. Key changes include the implementation of paragraph counting for chapters, which is now integrated into various upload and cloning scripts. Several new scripts and tools have been added to fix legacy HTML formats for both play and non-play books, converting custom tags and restructuring content. The AI processing pipeline is enhanced with a tool to restore unwrapped paragraphs from model output and updated prompts for better character handling. The player UI is also improved by fetching chapter titles from the backend instead of generating them on the client, and by gracefully handling unknown characters with a generic avatar. My feedback focuses on improving the robustness of a new Azure OpenAI client, fixing a hardcoded path in a script, and cleaning up some minor issues.
| const api_key = process.env.AZURE_GROK_KEY; | ||
|
|
||
| const client = new OpenAI({ baseURL: endpoint, apiKey: api_key }); |
There was a problem hiding this comment.
The api_key is read from process.env.AZURE_GROK_KEY but there's no check to ensure it's defined. If the environment variable is missing, api_key will be undefined, and the OpenAI client will be initialized with an invalid key. This will lead to runtime errors later on. It's better to fail fast with a clear error message if the key is not configured.
const api_key = process.env.AZURE_GROK_KEY;
if (!api_key) {
throw new Error("Missing environment variable: AZURE_GROK_KEY");
}
const client = new OpenAI({ baseURL: endpoint, apiKey: api_key });| // @ts-expect-error(zod typing) | ||
| schema: zodSchema.shape, |
There was a problem hiding this comment.
Using zodSchema.shape to generate the JSON schema is not robust. This property only exists on z.object schemas and doesn't correctly represent the full schema structure expected by the OpenAI API, especially for nested objects, arrays, or non-object types. This can lead to runtime errors or incorrect schema validation.
Consider using a library like zod-to-json-schema for a more reliable conversion. For example:
import { zodToJsonSchema } from 'zod-to-json-schema';
// ... inside callGrokAzureWithSchema
const jsonSchema = zodToJsonSchema(zodSchema);
const completion = await client.chat.completions.create({
// ...
response_format: {
type: "json_schema",
json_schema: {
name: "response",
strict: true,
schema: jsonSchema,
},
},
});This will require adding zod-to-json-schema as a dependency. The current implementation with @ts-expect-error is a strong indicator of a potential issue.
| const DEFAULT_OUTPUT_ROOT = | ||
| "/var/folders/j9/pbqwg7zs4336w7vccnz2xhcw0000gn/T/bookgenius-fixed-unwrapped"; |
There was a problem hiding this comment.
The DEFAULT_OUTPUT_ROOT is hardcoded to a specific user's temporary directory path on macOS. This makes the script non-portable and it will fail on any other machine or in a CI/CD environment. It should use a system-independent method for getting the temporary directory, like os.tmpdir(), which is used in other scripts in this PR.
| const DEFAULT_OUTPUT_ROOT = | |
| "/var/folders/j9/pbqwg7zs4336w7vccnz2xhcw0000gn/T/bookgenius-fixed-unwrapped"; | |
| import os from "os"; | |
| const DEFAULT_OUTPUT_ROOT = | |
| path.join(os.tmpdir(), "bookgenius-fixed-unwrapped"); |
| }); | ||
| let result: T; | ||
| try { | ||
| result = JSON.parse(completion.choices[0].message.content as string) as T; |
There was a problem hiding this comment.
The API response completion.choices[0].message.content can be null. Attempting to JSON.parse(null) will result in null, which might be acceptable, but if the model returns an empty response for some reason, it's better to handle this case explicitly to avoid unexpected behavior or provide a clearer error message.
const content = completion.choices[0].message.content;
if (content === null) {
throw new Error("Received null content from the model.");
}
result = JSON.parse(content) as T;|
|
||
| for (const file of rewrittenFiles) { | ||
| totalFiles += 1; | ||
| const chapter = Number(file.match(/(\d+)/)?.[1]); |
There was a problem hiding this comment.
The regex (\d+) used to extract the chapter number is a bit broad and could lead to incorrect parsing if the filename format changes slightly (e.g., includes other numbers). A more specific regex that anchors to the start and end of the expected pattern would be more robust.
| const chapter = Number(file.match(/(\d+)/)?.[1]); | |
| const chapter = Number(file.match(/^rewritten-paragraphs-for-chapter-(\d+)\.xml$/)?.[1]); |
| currentAct = getTitleText(actElements[0]); | ||
| } | ||
|
|
||
| console.log(`titleElements: ${titleElements.length}`); |
There was a problem hiding this comment.
This console.log statement appears to be a leftover from debugging. It should be removed to keep the codebase clean and avoid unnecessary output in production or CI logs.
📝 WalkthroughWalkthroughAdds Azure Grok client, paragraph-count utilities and DOM helpers, many HTML-normalization tools and CLI scripts for chapter processing, integrates paragraphCount into metadata flows and pipeline steps, expands image/avatar generation flows, and updates player UI handling for unknown characters and image zooming. Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant UploadFigures as UploadFiguresToConvex
participant FS as Filesystem
participant Convex
Pipeline->>UploadFigures: trigger upload_figures(job, repoRoot)
UploadFigures->>FS: list standardebooks metadata & images
FS-->>UploadFigures: image files
loop per image
UploadFigures->>Convex: uploadFile(path, content, contentType)
Convex-->>UploadFigures: upload result
UploadFigures->>Convex: update metadata (books/*/figures)
Convex-->>UploadFigures: ack
end
UploadFigures-->>Pipeline: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/player/src/components/modals/SearchModal.tsx (1)
34-54:⚠️ Potential issue | 🔴 CriticalGuard
bookDataaccess to prevent runtime crash if context hasn't loaded yet.
bookDatais dereferenced with a non-null assertion on line 372, butBookConvexContextinitializes it asnulland it remainsnulluntil book metadata finishes loading from Convex.SearchModalcan be opened beforebookDatais available, causing the assertion to fail at runtime. Use optional chaining instead:- chapterTitle={ - bookData!.chapters.find((c) => c.id === String(chapter))?.title ?? "" - } + chapterTitle={ + bookData?.chapters?.find((c) => c.id === String(chapter))?.title ?? "" + }Also applies to: 371–373
apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-summary.ts (1)
82-92:⚠️ Potential issue | 🟡 MinorHandle potential null response from
callGrokAzure.Based on the
callGrokAzureimplementation (returnscompletion.choices[0].message.content), the response can benullwhen the model returns no content. This null would propagate through and be cast as string on Line 100, potentially resulting in"null"being written to the summary file.Consider adding a null check after the provider call:
🛡️ Proposed fix
try { const summary = await selectedProvider( `${prompt}\n Reply in the language of the book. It's usually Polish or English. Your instructions are in English so you often reply in English, buts its VERY important to reply in Polish when the book is in Polish, and same goes for other languages.`, ); // Replace with actual LLM output + if (!summary) { + throw new Error("Empty response from LLM provider"); + } return summary; } catch (e) {apps/pipeline/src/tools/new-tooling/get-chapter-title.ts (1)
71-105:⚠️ Potential issue | 🟡 MinorRemove or gate the debug log.
Theconsole.login the legacy path will add noise to pipeline runs.🧹 Suggested removal
- console.log(`titleElements: ${titleElements.length}`); - const titleText = getTitleText(titleElements[0]);
🤖 Fix all issues with AI agents
In `@apps/pipeline/src/callGrokAzure.ts`:
- Around line 6-8: Ensure AZURE_GROK_KEY is validated before creating the OpenAI
client: check process.env.AZURE_GROK_KEY (the api_key variable) and if it's
missing/empty throw a clear Error or log and exit, then only instantiate new
OpenAI({ baseURL: endpoint, apiKey: api_key }); that way OpenAI client creation
(the client variable) never occurs with an undefined key and failure happens at
startup instead of at first API call.
- Around line 10-17: callGrokAzure currently returns
completion.choices[0].message.content which can be null; update callGrokAzure to
handle that case by checking completion and
completion.choices[0].message.content and either (a) return a safe fallback
string (e.g., empty string) or (b) throw a clear error, and update the function
signature accordingly to reflect string | null or throw; reference the symbols
callGrokAzure, completion, choices, and message.content when making the change
so the null-check covers completion.choices[0] and message.content before
returning.
In `@apps/pipeline/src/scripts/fix-legacy-play-chapters.ts`:
- Around line 22-44: The parseArgs function currently assumes a value follows
--source or --out, causing resolvePath to throw if the flag is present without a
value; update parseArgs to validate that when sourceIdx or outIdx !== -1 there
is an args[sourceIdx+1] / args[outIdx+1] and that it is not another flag (e.g.,
doesn't start with "--"); if validation fails, print a clear usage/error
mentioning bookSlug and the missing flag and exit(1); otherwise pass the
validated value to resolvePath as before (keep using defaultSource/defaultOut
when the flag is absent).
In `@apps/pipeline/src/scripts/fix-non-play-chapters.ts`:
- Around line 27-45: The parseArgs function currently trusts that flags
(--source, --out, --slugs) always have a following value which can result in
undefined being passed into resolvePath; update parseArgs to validate that
args[sourceIdx + 1], args[outIdx + 1], and args[slugsIdx + 1] exist and are not
another flag (e.g., start with "-") before using them; if a flag is present but
its value is missing, throw an explicit error or exit with a clear message so we
never call resolvePath with undefined; adjust the slugs parsing to only split
when a valid value is present and keep existing dryRun handling unchanged
(references: parseArgs, sourceIdx, outIdx, slugsIdx, resolvePath).
In `@apps/pipeline/src/scripts/fix-unwrapped-paragraphs-in-temporary-output.ts`:
- Around line 60-62: The current concatenation in the return statement uses
chunkFiles.map(...).join("\n") which can add unwanted newlines between chunks;
update the code that builds the final string (the map over chunkFiles that reads
from path.join(tempDir, entry.file)) to join with an empty string (use .join("")
) or otherwise preserve the original chunk boundaries (e.g., respect an explicit
separator stored on each entry) so you don't introduce extra whitespace between
files; ensure you reference chunkFiles, tempDir, and entry.file when making the
change.
In `@apps/pipeline/src/scripts/scan-non-html-tags.ts`:
- Around line 7-112: Rename the constant HTML_TAGS to camelCase (e.g., htmlTags)
everywhere in this file: update the declaration (const HTML_TAGS = new
Set([...])) to const htmlTags = new Set([...]) and then update all
references/usages of HTML_TAGS (including the similar occurrences later in the
file noted by the reviewer) to use htmlTags so the identifier follows the repo's
camelCase naming conventions.
- Around line 121-143: The CLI argument parsing in parseArgs may pass undefined
into resolvePath and produce NaN for limit when flags are provided without
values; fix parseArgs by validating that for each flag (check args[sourceIdx +
1], args[limitIdx + 1], args[slugsIdx + 1]) the next token exists and is not
another flag (doesn't start with "--"), otherwise fall back to the
defaultSource/default limit and empty slugs behavior; also parse limit with
Number or parseInt and if result is NaN use the default (5) and ensure slugs
handling only splits when a non-empty value is present before calling split and
map so resolvePath, sourceRoot, limit, and slugs are never assigned invalid
values.
In `@apps/pipeline/src/scripts/upload-chapters-source.ts`:
- Around line 6-8: Replace the absolute import of getChapterTitle with a
relative import to follow the pipeline app convention: change the import of
getChapterTitle (currently "src/tools/new-tooling/get-chapter-title") to the
sibling relative path from this file (e.g.,
"../tools/new-tooling/get-chapter-title") so it matches other imports like
computeParagraphCount and keeps consistency in upload-chapters-source.ts.
In `@apps/pipeline/src/scripts/upload-fixed-nonplays.ts`:
- Around line 32-47: In parseArgs(), guard accesses to args[sourceIdx + 1] and
args[slugsIdx + 1] so missing flag values don't crash resolvePath or split;
specifically, when sourceIdx !== -1 ensure args[sourceIdx + 1] exists and is
non-empty (otherwise use defaultSource or throw a clear error) before calling
resolvePath; when slugsIdx !== -1 ensure args[slugsIdx + 1] exists before
splitting (otherwise set slugs to [] or null consistently); update the logic
that builds sourceRoot and slugs in parseArgs to check for the presence and
non-empty value at args[index+1] rather than assuming it exists.
In `@apps/pipeline/src/server/regenerate-missing-avatars.ts`:
- Around line 172-182: The empty catch around
getPublishedFilesInFolder(genericPath) swallows all errors and can mask
auth/network failures; either remove the try-catch if getPublishedFilesInFolder
is guaranteed to return [] for missing folders, or replace the empty catch with
the same error extraction/handling used later (use const errorMsg = e instanceof
Error ? e.message : String(e), log the error via console.error/processLogger and
return or rethrow for unexpected errors). Also apply the same handling to the
other places that call getPublishedFilesInFolder (the earlier loop) so transient
failures are logged and do not silently trigger avatar generation.
In `@apps/pipeline/src/tools/new-tooling/generate-flux-schnel-image.ts`:
- Line 11: The import of logError in generate-flux-schnel-image.ts is using an
implicit wildcard path mapping; update it to use an explicit path or relative
import to make resolution maintainable: either add a "src/*": ["./*"] entry to
apps/pipeline/tsconfig.json so import { logError } from "src/helpers/logError"
is explicit, or change the import in generate-flux-schnel-image.ts to a relative
path (e.g., ../../helpers/logError) so the symbol logError resolves without
relying on the wildcard mapping.
- Around line 154-183: The output-handling around
replicate.run("black-forest-labs/flux-2-klein-4b") is fragile and contains a
mistaken null-check (`if (!url!)`) and relies on exceptions instead of type
guards; replace the nested try/catches with explicit shape checks for output
(e.g., if typeof output === "string" -> url = output; else if
Array.isArray(output) and first element has url() -> url = array[0].url(); else
if output && typeof output.url === "function" -> url = output.url()) and then do
a proper null-check `if (!url) throw new Error("Failed to get URL from
output")`. Also review and adjust the input object fields `output_megapixels`
and `disable_safety_checker` in the `input` passed to replicate.run to match the
flux-2-klein-4b Replicate schema (remove or rename unsupported fields), and
ensure you log and rethrow errors from replicate.run using the existing logError
helper when shape checks fail.
In
`@apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts`:
- Line 10: The import for callGrokAzureWithSchema is using an absolute-style
specifier "src/callGrokAzure" which may not resolve; update the import to the
correct relative path used by this module (e.g., change the module specifier to
the proper ../../ or ../ relative path that matches other imports in this file)
so callGrokAzureWithSchema resolves at runtime and the symbol can be imported
successfully.
In `@apps/pipeline/src/tools/new-tooling/section-wrapper.ts`:
- Around line 52-58: In buildAttributeString, the current regex /\\"/g only
matches a backslash+quote sequence so plain double quotes in attribute values
aren't escaped; update the replacement to use /"/g (i.e., value.replace(/"/g,
""")) so all double-quote characters are converted to " when building
the attribute string.
In `@apps/player/src/components/modals/CharacterModal.tsx`:
- Around line 54-58: The computed chapterTitle uses bookData! and can throw
during initial load; update the useMemo in CharacterModal (symbols:
useBookConvex, bookData, chapterTitle) to guard bookData before accessing
chapters by using optional chaining and a safe fallback (e.g., null or an empty
string) so chapterTitle is computed only when bookData is present and never
dereferences null during render.
- Around line 188-226: The unknown-character modal's close control
(DialogEnhanceClose) only handles pointer events—add keyboard support to match
the main modal by handling onKeyDown for Enter/Space and ensuring it calls
handleOnClose; update the DialogEnhanceClose element in the unknown-character
branch to include an onKeyDown handler that prevents default/propagation for
Enter (key === "Enter") and Space (key === " " or key === "Spacebar") and then
invokes handleOnClose, and ensure the element is focusable (e.g., has
tabIndex={0} and appropriate aria-label) so keyboard users can reach it.
🧹 Nitpick comments (18)
apps/pipeline/src/scripts/fix-unwrapped-paragraphs-in-temporary-output.ts (1)
7-8: Hardcoded machine-specific temp path.The
DEFAULT_OUTPUT_ROOTcontains a machine-specific path that won't work on other systems. Consider using a portable temp directory or a path relative to the project.♻️ Suggested fix using OS temp directory
+import os from "os"; + -const DEFAULT_OUTPUT_ROOT = - "/var/folders/j9/pbqwg7zs4336w7vccnz2xhcw0000gn/T/bookgenius-fixed-unwrapped"; +const DEFAULT_OUTPUT_ROOT = path.join(os.tmpdir(), "bookgenius-fixed-unwrapped");apps/player/src/helpers/minorCharacterUtils.ts (1)
19-23: Consider filtering empty slug segments for cleaner names.Consecutive or trailing hyphens can introduce empty words and extra spaces. Filtering empties keeps display names tidy.
♻️ Optional refinement
export function slugToDisplayName(slug: string): string { - return slug - .split("-") - .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) - .join(" "); + const words = slug.split("-").filter(Boolean); + if (words.length === 0) return slug; + return words.map((word) => word.charAt(0).toUpperCase() + word.slice(1)).join(" "); }apps/pipeline/package.json (1)
23-23: Consider upgrading@ai-sdk/azureto the latest version.Version
2.0.91exists and has no known security vulnerabilities. However, the latest available version is3.0.26. Review the changelog between these major versions to determine if an upgrade is feasible and beneficial for your use case.apps/pipeline/src/tools/fix-legacy-play-stage-directions.ts (2)
44-49: Consider extracting shared helpers to reduce duplication.The
slugifyTagandcopyAttributesfunctions are duplicated between this file andfix-legacy-play-custom-tags.ts. Consider extracting them into a shared utility module (e.g.,html-transform-utils.ts) to maintain DRY principles.♻️ Suggested shared module
Create a new file
apps/pipeline/src/tools/html-transform-utils.ts:export function slugifyTag(tagName: string): string { return tagName .toLowerCase() .replace(/[^a-z0-9]+/g, "-") .replace(/^-+|-+$/g, ""); } export function copyAttributes(target: Element, source: Element, skip: Set<string>): void { for (const attr of Array.from(source.attributes)) { const name = attr.name.toLowerCase(); if (skip.has(name)) continue; target.setAttribute(attr.name, attr.value); } }Then import in both files:
import { slugifyTag, copyAttributes } from "./html-transform-utils";Also applies to: 63-69
58-61: Simplify:getAttrValuewrapper is unnecessary.
element.getAttribute(name)already returnsnullwhen the attribute doesn't exist. ThehasAttributecheck adds no value here.♻️ Simplified usage
-function getAttrValue(element: Element, name: string): string | null { - if (element.hasAttribute(name)) return element.getAttribute(name); - return null; -} - -// Usage: -const entersValue = getAttrValue(element, "enters") ?? getAttrValue(element, "data-enters"); +// Direct usage: +const entersValue = element.getAttribute("enters") ?? element.getAttribute("data-enters");apps/pipeline/src/tools/fix-legacy-play-custom-tags.ts (1)
145-149: Minor: Consider hoisting the skip set to module scope.The skip
Setis recreated on every function call. For a frequently-called function, hoisting it to module scope would be a minor optimization.♻️ Optional optimization
+const SKIP_ATTRS = new Set(["enters", "exits", "data-enters", "data-exits", "talking"]); + export function fixLegacyPlayCustomTags(html: string): string { // ... copyAttributes( span, element, - new Set(["enters", "exits", "data-enters", "data-exits", "talking"]), + SKIP_ATTRS, );apps/pipeline/src/tools/fix-legacy-play-custom-tags.spec.ts (1)
16-35: Minor: Test description mentions "outside em" but implementation doesn't check this.The test description says "converts non-HTML tags outside em" but
fixLegacyPlayCustomTagsconverts all non-HTML tags regardless of theiremcontext (unlikefixLegacyPlayStageDirectionswhich specifically checks foremancestors).Consider updating the description to avoid confusion:
📝 Suggested description update
- it("converts non-HTML tags outside em to spans with data-c", () => { + it("converts non-HTML tags to spans with data-c", () => {apps/player/src/services/htmlNormalizer.ts (1)
770-809: Optional: avoid double parsing when counting data-index.
countParagraphsFromChapterHtmlnormalizes to HTML and then re-parses it to count. If this runs in batch (pipeline backfills), consider returning the count from the normalization step or allowingcountDataIndexFromHtmlto accept aDocumentto reuse the already-parsed DOM.apps/pipeline/src/tools/fix-legacy-play-didaskalia.ts (1)
3-3: Use camelCase for constant naming.
Consider renamingBOUNDARY_TAGStoboundaryTagsto match the camelCase guideline.As per coding guidelines: Use camelCase for variable and function naming.♻️ Proposed rename
-const BOUNDARY_TAGS = new Set(["h1", "h2", "h3", "h4", "h5", "h6"]); +const boundaryTags = new Set(["h1", "h2", "h3", "h4", "h5", "h6"]);- return BOUNDARY_TAGS.has(tagName) || tagName === "section"; + return boundaryTags.has(tagName) || tagName === "section";apps/pipeline/src/scripts/upload-fixed-nonplays.ts (1)
14-23: Use camelCase for constant naming.
RenamePLAY_SLUGStoplaySlugsto align with the naming guideline.As per coding guidelines: Use camelCase for variable and function naming.♻️ Proposed rename
-const PLAY_SLUGS = new Set([ +const playSlugs = new Set([ "Hamlet", "Macbeth", "Midsummer-Nights-Dream", "Othello", "Romeo-And-Juliet", "The-Tempest", "Romeo-And-Juliet-Small", "Romeo-And-Juliet-Smaller", ]);- if (PLAY_SLUGS.has(slug)) continue; + if (playSlugs.has(slug)) continue;apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts (1)
230-245: Consider adding chunking for long chapters to respect LLM context limits.The chapter content is sent to the LLM as-is without checking length. Per pipeline guidelines, long chapters should be split into chunks when processing with LLM to respect context limits. Very long chapters could exceed the model's context window.
Based on learnings: "Split long chapters into chunks when processing with LLM to respect context limits".
apps/pipeline/src/scripts/upload-chapters-source.spec.ts (1)
1-14: Good test coverage for happy paths; consider adding error case.The tests cover both valid filename patterns. Consider adding a test for the error case when an invalid filename is provided, to ensure the function throws as expected.
💡 Suggested additional test
+test("mapFilenameToBasename throws for invalid filename", () => { + expect(() => mapFilenameToBasename("invalid-file.txt")).toThrow("Invalid filename"); +});apps/pipeline/src/callGrokAzure.ts (1)
19-41: Update function signature to match the AI wrapper interface pattern and fix JSON schema conversion.Per coding guidelines for
apps/pipeline/src/call*.ts, this function should:
- Accept an optional
maxRetriesparameter (default 3) matching the pattern used incallClaudeandcallActualChatGPT- Return
Promise<T | string>to match the shared interface specification inapps/pipeline/AGENTS.md- Use
zodToJsonSchema()instead ofzodSchema.shapeto properly convert Zod schemas to valid OpenAI JSON Schema format (thezod-to-json-schemalibrary is already in your project dependencies)♻️ Suggested approach
import OpenAI from "openai"; import { type z } from "zod"; +import { zodToJsonSchema } from "zod-to-json-schema"; const endpoint = "https://bookgenius.services.ai.azure.com/openai/v1/"; const model = "grok-4-fast-reasoning"; const api_key = process.env.AZURE_GROK_KEY; const client = new OpenAI({ baseURL: endpoint, apiKey: api_key }); -export const callGrokAzureWithSchema = async <T>(prompt: string, zodSchema: z.ZodSchema<T>) => { +export const callGrokAzureWithSchema = async <T>( + prompt: string, + zodSchema: z.ZodSchema<T>, + maxRetries = 3, +): Promise<T | string> => { const completion = await client.chat.completions.create({ messages: [{ role: "user", content: prompt }], model, response_format: { type: "json_schema", json_schema: { name: "response", strict: true, - // `@ts-expect-error`(zod typing) - schema: zodSchema.shape, + schema: zodToJsonSchema(zodSchema), }, }, }); let result: T; try { result = JSON.parse(completion.choices[0].message.content as string) as T; } catch (e) { console.error("Error parsing JSON", e); throw e; } return result; };apps/pipeline/src/scripts/upload-chapters-source.ts (2)
171-183: Variable shadowing: innermatchshadows outermatch.The inner
matchvariable on line 176 shadows the outermatchfrom line 172. This works but reduces readability.♻️ Suggested refactor
export function mapFilenameToBasename(filename: string): string { - const match = filename.match(/^rewritten-paragraphs-for-chapter-(\d+)\.xml$/); - if (match) { - return `chapter-${match[1]}.html`; + const xmlMatch = filename.match(/^rewritten-paragraphs-for-chapter-(\d+)\.xml$/); + if (xmlMatch) { + return `chapter-${xmlMatch[1]}.html`; } else { - const match = filename.match(/^chapter-(\d+)\.html$/); - if (match) { - return `chapter-${match[1]}.html`; + const htmlMatch = filename.match(/^chapter-(\d+)\.html$/); + if (htmlMatch) { + return `chapter-${htmlMatch[1]}.html`; } else { throw new Error(`Invalid filename: ${filename}`); } } }
164-169: Replace xmldom DOMParser with jsdom for consistency.The
parseChapterIntoDomfunction uses@xmldom/xmldom's DOMParser with"text/html", which is inconsistent with the codebase. JSDOM is already a dependency and is used throughout the pipeline app (e.g.,getChapterFormat.ts,se-converter/index.ts,fix-legacy-play-didaskalia.ts) for HTML parsing. Update this function to use JSDOM like the rest of the codebase.apps/pipeline/src/scripts/fix-non-play-chapters.ts (1)
9-18: Use camelCase for constant naming.
RenamePLAY_SLUGStoplaySlugs(and update usage) to align with project naming rules.As per coding guidelines, Use camelCase for variable and function naming.♻️ Suggested rename
-const PLAY_SLUGS = new Set([ +const playSlugs = new Set([ "Hamlet", "Macbeth", "Midsummer-Nights-Dream", "Othello", "Romeo-And-Juliet", "The-Tempest", "Romeo-And-Juliet-Small", "Romeo-And-Juliet-Smaller", ]); @@ - if (PLAY_SLUGS.has(slug)) continue; + if (playSlugs.has(slug)) continue;Also applies to: 73-73
apps/pipeline/src/tools/new-tooling/get-chapter-title.ts (1)
5-13: Prefer tokenizeddata-epub-typematching.
includes()can match partial strings; splitting on whitespace avoids false positives when multiple types are present.♻️ Suggested update
const hasEpubType = (el: XMLElement, type: string): boolean => { const epubType = getAttribute(el, "data-epub-type"); - return epubType ? epubType.includes(type) : false; + return epubType ? epubType.split(/\s+/).includes(type) : false; };apps/pipeline/src/tools/fix-non-play-custom-tags.ts (1)
3-113: Use camelCase for constant identifiers.
RenameHTML_TAGS,IGNORED_CUSTOM_TAGS, andNON_SPEAKER_TAGSto camelCase and update usages.As per coding guidelines, Use camelCase for variable and function naming.♻️ Suggested rename
-const HTML_TAGS = new Set([ +const htmlTags = new Set([ @@ -const IGNORED_CUSTOM_TAGS = new Set(["hgroup"]); -const NON_SPEAKER_TAGS = new Set(["note", "see"]); +const ignoredCustomTags = new Set(["hgroup"]); +const nonSpeakerTags = new Set(["note", "see"]); @@ - if (HTML_TAGS.has(lower)) return false; - if (IGNORED_CUSTOM_TAGS.has(lower)) return false; + if (htmlTags.has(lower)) return false; + if (ignoredCustomTags.has(lower)) return false; @@ - if (NON_SPEAKER_TAGS.has(tagName)) continue; + if (nonSpeakerTags.has(tagName)) continue; @@ - if (IGNORED_CUSTOM_TAGS.has(tagName)) continue; + if (ignoredCustomTags.has(tagName)) continue;
| const api_key = process.env.AZURE_GROK_KEY; | ||
|
|
||
| const client = new OpenAI({ baseURL: endpoint, apiKey: api_key }); |
There was a problem hiding this comment.
Missing API key validation before client initialization.
If AZURE_GROK_KEY is undefined, the OpenAI client will be created with an undefined API key, which will fail at runtime during the first API call rather than at module load time. Consider adding validation.
🛡️ Proposed fix
const api_key = process.env.AZURE_GROK_KEY;
+
+if (!api_key) {
+ throw new Error("Missing AZURE_GROK_KEY environment variable");
+}
const client = new OpenAI({ baseURL: endpoint, apiKey: api_key });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const api_key = process.env.AZURE_GROK_KEY; | |
| const client = new OpenAI({ baseURL: endpoint, apiKey: api_key }); | |
| const api_key = process.env.AZURE_GROK_KEY; | |
| if (!api_key) { | |
| throw new Error("Missing AZURE_GROK_KEY environment variable"); | |
| } | |
| const client = new OpenAI({ baseURL: endpoint, apiKey: api_key }); |
🤖 Prompt for AI Agents
In `@apps/pipeline/src/callGrokAzure.ts` around lines 6 - 8, Ensure AZURE_GROK_KEY
is validated before creating the OpenAI client: check process.env.AZURE_GROK_KEY
(the api_key variable) and if it's missing/empty throw a clear Error or log and
exit, then only instantiate new OpenAI({ baseURL: endpoint, apiKey: api_key });
that way OpenAI client creation (the client variable) never occurs with an
undefined key and failure happens at startup instead of at first API call.
| export const callGrokAzure = async (prompt: string) => { | ||
| const completion = await client.chat.completions.create({ | ||
| messages: [{ role: "user", content: prompt }], | ||
| model, | ||
| }); | ||
|
|
||
| return completion.choices[0].message.content; | ||
| }; |
There was a problem hiding this comment.
Return type may be null; consider handling.
completion.choices[0].message.content can be null when the model returns a refusal or empty response. The current signature implies it always returns a string.
🛡️ Proposed fix to handle null content
-export const callGrokAzure = async (prompt: string) => {
+export const callGrokAzure = async (prompt: string): Promise<string> => {
const completion = await client.chat.completions.create({
messages: [{ role: "user", content: prompt }],
model,
});
- return completion.choices[0].message.content;
+ const content = completion.choices[0].message.content;
+ if (content === null) {
+ throw new Error("Model returned null content");
+ }
+ return content;
};🤖 Prompt for AI Agents
In `@apps/pipeline/src/callGrokAzure.ts` around lines 10 - 17, callGrokAzure
currently returns completion.choices[0].message.content which can be null;
update callGrokAzure to handle that case by checking completion and
completion.choices[0].message.content and either (a) return a safe fallback
string (e.g., empty string) or (b) throw a clear error, and update the function
signature accordingly to reflect string | null or throw; reference the symbols
callGrokAzure, completion, choices, and message.content when making the change
so the null-check covers completion.choices[0] and message.content before
returning.
| function parseArgs(): Args { | ||
| const args = process.argv.slice(2); | ||
| if (args.length < 1) { | ||
| console.error( | ||
| "Usage: bun apps/pipeline/src/scripts/fix-legacy-play-chapters.ts <book-slug> [--source <dir>] [--out <dir>] [--dry-run]", | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const bookSlug = args[0]; | ||
| const sourceIdx = args.indexOf("--source"); | ||
| const outIdx = args.indexOf("--out"); | ||
|
|
||
| const repoRoot = path.resolve(process.cwd()); | ||
| const defaultSource = path.join(repoRoot, "ConvexAssets", "books", bookSlug, "chapters-source"); | ||
| const defaultOut = path.join(os.tmpdir(), "bookgenius-fixed-chapters", bookSlug); | ||
|
|
||
| return { | ||
| bookSlug, | ||
| sourceDir: resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource), | ||
| outputDir: resolvePath(outIdx !== -1 ? args[outIdx + 1] : defaultOut), | ||
| dryRun: args.includes("--dry-run"), | ||
| }; |
There was a problem hiding this comment.
Validate --source / --out values to prevent crashes.
If the flag is present but missing a value, resolvePath will throw.
🛡️ Suggested validation
const bookSlug = args[0];
const sourceIdx = args.indexOf("--source");
const outIdx = args.indexOf("--out");
@@
- return {
+ const sourceValue = sourceIdx !== -1 ? args[sourceIdx + 1] : undefined;
+ if (sourceIdx !== -1 && !sourceValue) {
+ console.error("Missing value for --source");
+ process.exit(1);
+ }
+
+ const outValue = outIdx !== -1 ? args[outIdx + 1] : undefined;
+ if (outIdx !== -1 && !outValue) {
+ console.error("Missing value for --out");
+ process.exit(1);
+ }
+
+ return {
bookSlug,
- sourceDir: resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource),
- outputDir: resolvePath(outIdx !== -1 ? args[outIdx + 1] : defaultOut),
+ sourceDir: resolvePath(sourceIdx !== -1 ? sourceValue : defaultSource),
+ outputDir: resolvePath(outIdx !== -1 ? outValue : defaultOut),
dryRun: args.includes("--dry-run"),
};
}🤖 Prompt for AI Agents
In `@apps/pipeline/src/scripts/fix-legacy-play-chapters.ts` around lines 22 - 44,
The parseArgs function currently assumes a value follows --source or --out,
causing resolvePath to throw if the flag is present without a value; update
parseArgs to validate that when sourceIdx or outIdx !== -1 there is an
args[sourceIdx+1] / args[outIdx+1] and that it is not another flag (e.g.,
doesn't start with "--"); if validation fails, print a clear usage/error
mentioning bookSlug and the missing flag and exit(1); otherwise pass the
validated value to resolvePath as before (keep using defaultSource/defaultOut
when the flag is absent).
| function parseArgs(): Args { | ||
| const args = process.argv.slice(2); | ||
| const sourceIdx = args.indexOf("--source"); | ||
| const outIdx = args.indexOf("--out"); | ||
| const slugsIdx = args.indexOf("--slugs"); | ||
|
|
||
| const repoRoot = path.resolve(process.cwd()); | ||
| const defaultSource = path.join(repoRoot, "ConvexAssets", "books"); | ||
| const defaultOut = path.join(os.tmpdir(), "bookgenius-fixed-nonplays"); | ||
|
|
||
| const slugs = | ||
| slugsIdx !== -1 ? (args[slugsIdx + 1]?.split(",").map((s) => s.trim()) ?? []) : null; | ||
|
|
||
| return { | ||
| sourceRoot: resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource), | ||
| outputRoot: resolvePath(outIdx !== -1 ? args[outIdx + 1] : defaultOut), | ||
| slugs, | ||
| dryRun: args.includes("--dry-run"), | ||
| }; |
There was a problem hiding this comment.
Validate missing CLI flag values.
If a flag is provided without a value, the current logic resolves to an unintended path (e.g., undefined), which can lead to accidental reads/writes.
🛠️ Suggested guard
function parseArgs(): Args {
const args = process.argv.slice(2);
const sourceIdx = args.indexOf("--source");
const outIdx = args.indexOf("--out");
const slugsIdx = args.indexOf("--slugs");
@@
- return {
- sourceRoot: resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource),
- outputRoot: resolvePath(outIdx !== -1 ? args[outIdx + 1] : defaultOut),
+ const readValue = (flag: string, idx: number, fallback: string): string => {
+ if (idx === -1) return fallback;
+ const value = args[idx + 1];
+ if (!value || value.startsWith("--")) {
+ throw new Error(`Missing value for ${flag}`);
+ }
+ return value;
+ };
+
+ return {
+ sourceRoot: resolvePath(readValue("--source", sourceIdx, defaultSource)),
+ outputRoot: resolvePath(readValue("--out", outIdx, defaultOut)),
slugs,
dryRun: args.includes("--dry-run"),
};
}🤖 Prompt for AI Agents
In `@apps/pipeline/src/scripts/fix-non-play-chapters.ts` around lines 27 - 45, The
parseArgs function currently trusts that flags (--source, --out, --slugs) always
have a following value which can result in undefined being passed into
resolvePath; update parseArgs to validate that args[sourceIdx + 1], args[outIdx
+ 1], and args[slugsIdx + 1] exist and are not another flag (e.g., start with
"-") before using them; if a flag is present but its value is missing, throw an
explicit error or exit with a clear message so we never call resolvePath with
undefined; adjust the slugs parsing to only split when a valid value is present
and keep existing dryRun handling unchanged (references: parseArgs, sourceIdx,
outIdx, slugsIdx, resolvePath).
| return chunkFiles | ||
| .map((entry) => fs.readFileSync(path.join(tempDir, entry.file), "utf-8")) | ||
| .join("\n"); |
There was a problem hiding this comment.
Chunk concatenation may introduce extra whitespace.
Joining chunk contents with "\n" could introduce unintended newlines between chunks, potentially affecting the restoration logic if whitespace is significant. Consider joining with an empty string or ensuring the chunks have proper boundaries.
🔧 Suggested fix
return chunkFiles
.map((entry) => fs.readFileSync(path.join(tempDir, entry.file), "utf-8"))
- .join("\n");
+ .join("");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return chunkFiles | |
| .map((entry) => fs.readFileSync(path.join(tempDir, entry.file), "utf-8")) | |
| .join("\n"); | |
| return chunkFiles | |
| .map((entry) => fs.readFileSync(path.join(tempDir, entry.file), "utf-8")) | |
| .join(""); |
🤖 Prompt for AI Agents
In `@apps/pipeline/src/scripts/fix-unwrapped-paragraphs-in-temporary-output.ts`
around lines 60 - 62, The current concatenation in the return statement uses
chunkFiles.map(...).join("\n") which can add unwanted newlines between chunks;
update the code that builds the final string (the map over chunkFiles that reads
from path.join(tempDir, entry.file)) to join with an empty string (use .join("")
) or otherwise preserve the original chunk boundaries (e.g., respect an explicit
separator stored on each entry) so you don't introduce extra whitespace between
files; ensure you reference chunkFiles, tempDir, and entry.file when making the
change.
| const input = { | ||
| images: [], | ||
| prompt: finalPrompt, | ||
| go_fast: false, | ||
| aspect_ratio: type === "avatar" ? "1:1" : "16:9", | ||
| input_images: [], | ||
| output_format: type === "background" ? "webp" : "png", | ||
| output_quality: 80, | ||
| prompt: finalPrompt, | ||
| resolution: "1 MP", | ||
| safety_tolerance: 5, | ||
| seed: 43605, | ||
| output_megapixels: "1", | ||
| disable_safety_checker: true, | ||
| }; | ||
|
|
||
| try { | ||
| const output = await replicate.run("black-forest-labs/flux-2-pro", { input }); | ||
| const output = await replicate.run("black-forest-labs/flux-2-klein-4b", { input }); | ||
|
|
||
| // @ts-expect-error wrong types of replicate - flux-2-pro returns object with .url() method | ||
| const url = output.url(); | ||
| let url: string; | ||
| try { | ||
| // @ts-expect-error wrong types of replicate - flux-2-pro returns object with .url() method | ||
| url = output.url(); | ||
| } catch { | ||
| try { | ||
| url = (output as unknown as { url: () => string }[])[0].url(); | ||
| } catch (e) { | ||
| logError("Failed to get URL from output", e); | ||
| } | ||
| } | ||
|
|
||
| if (!url!) { | ||
| throw new Error("Failed to get URL from output"); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Replicate black-forest-labs/flux-2-klein-4b model input output schema API
💡 Result:
To replicate the input/output schema for black-forest-labs/flux-2-klein-4b on Replicate, fetch the model’s OpenAPI schema from Replicate’s REST API and read the openapi_schema object. Replicate exposes per-model schemas via models.get (or per-version via models.versions.get). [1], [2], [3]
1) Get the latest-version schema (recommended)
curl -s \
-H "Authorization: Bearer $REPLICATE_API_TOKEN" \
https://api.replicate.com/v1/models/black-forest-labs/flux-2-klein-4b \
| jq '.latest_version.openapi_schema.components.schemas
| {Input, Output}'Replicate documents that this returns OpenAPI Schema Objects for Input/Output under latest_version.openapi_schema. [1], [2]
2) Get schema for a specific version (pin reproducibility)
# First, get the latest version id:
VERSION=$(curl -s -H "Authorization: Bearer $REPLICATE_API_TOKEN" \
https://api.replicate.com/v1/models/black-forest-labs/flux-2-klein-4b \
| jq -r '.latest_version.id')
# Then fetch that version’s schema:
curl -s \
-H "Authorization: Bearer $REPLICATE_API_TOKEN" \
"https://api.replicate.com/v1/models/black-forest-labs/flux-2-klein-4b/versions/$VERSION" \
| jq '.openapi_schema.components.schemas | {Input, Output}'Replicate notes you can fetch the schema from a model version object via openapi_schema. [2]
3) Extract only input (or only output) fields
# Input properties only
... | jq '.latest_version.openapi_schema.components.schemas.Input.properties'
# Output schema only
... | jq '.latest_version.openapi_schema.components.schemas.Output'This is the same pattern Replicate shows in their OpenAPI reference. [1]
Sources: Replicate model page [3]; Replicate OpenAPI/schema docs [1]; Replicate changelog explaining openapi_schema for input/output [2].
🏁 Script executed:
# First, let's examine the actual file to understand the full context
cat -n apps/pipeline/src/tools/new-tooling/generate-flux-schnel-image.ts | head -200Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 7317
🏁 Script executed:
# Check for any documentation or comments about Replicate API usage in the codebase
rg "replicate" --type ts --type tsx -i -A 3 -B 3 | head -100Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 102
🏁 Script executed:
# Search for other Replicate API calls to see common patterns
rg "replicate.run" --type ts -B 5 -A 10Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 5040
🏁 Script executed:
# Search for any tests related to this file or replicate
fd "test\|spec" -e ts -e tsx | xargs grep -l "flux\|replicate" 2>/dev/null || echo "No test files found with flux or replicate"Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 117
🏁 Script executed:
# Check if there's any documentation about Replicate models in the codebase
find . -name "*.md" -type f | xargs grep -l "replicate\|flux" 2>/dev/null || echo "No markdown docs found"Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 149
🏁 Script executed:
# Look for recent git commits or comments about this specific output handling
rg "flux-2-klein-4b" --type tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 228
🏁 Script executed:
# Check for any type definitions or interfaces related to replicate output
rg "type.*[Rr]eplicate\|interface.*[Rr]eplicate" --type tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 57
🏁 Script executed:
# Check the type definition for ReplicateOutput
rg "ReplicateOutput" --type ts -B 5 -A 5Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3535
🏁 Script executed:
# Look at the broader context of generate-flux-schnel-image.ts for error handling patterns
sed -n '140,200p' apps/pipeline/src/tools/new-tooling/generate-flux-schnel-image.tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1790
🏁 Script executed:
# Search for any documentation or comments about flux models
rg "flux" --type ts --type md -i -B 2 -A 2 | head -150Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 13220
Fix output handling logic and type safety for flux-2-klein-4b model.
The current code has a critical bug at line 180: if (!url!) should be if (!url). More importantly, the nested try-catch approach is fragile and allows undefined url to potentially reach the error check after silent failures.
The output shape handling lacks proper type guards. Implement explicit checks for different possible output formats (string, string array, or object with url method) rather than relying on exception handling. Additionally, verify that the input parameters output_megapixels and disable_safety_checker are valid for the flux-2-klein-4b model per its Replicate schema, as they differ significantly from the commented flux-2-pro parameters.
🛠️ Suggested fix
- let url: string;
- try {
- // `@ts-expect-error` wrong types of replicate - flux-2-pro returns object with .url() method
- url = output.url();
- } catch {
- try {
- url = (output as unknown as { url: () => string }[])[0].url();
- } catch (e) {
- logError("Failed to get URL from output", e);
- }
- }
-
- if (!url!) {
+ const url =
+ typeof output === "string"
+ ? output
+ : Array.isArray(output)
+ ? typeof output[0] === "string"
+ ? output[0]
+ : output[0]?.url?.()
+ : (output as { url?: () => string })?.url?.();
+
+ if (!url) {
throw new Error("Failed to get URL from output");
}🤖 Prompt for AI Agents
In `@apps/pipeline/src/tools/new-tooling/generate-flux-schnel-image.ts` around
lines 154 - 183, The output-handling around
replicate.run("black-forest-labs/flux-2-klein-4b") is fragile and contains a
mistaken null-check (`if (!url!)`) and relies on exceptions instead of type
guards; replace the nested try/catches with explicit shape checks for output
(e.g., if typeof output === "string" -> url = output; else if
Array.isArray(output) and first element has url() -> url = array[0].url(); else
if output && typeof output.url === "function" -> url = output.url()) and then do
a proper null-check `if (!url) throw new Error("Failed to get URL from
output")`. Also review and adjust the input object fields `output_megapixels`
and `disable_safety_checker` in the `input` passed to replicate.run to match the
flux-2-klein-4b Replicate schema (remove or rename unsupported fields), and
ensure you log and rethrow errors from replicate.run using the existing logError
helper when shape checks fail.
apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts
Outdated
Show resolved
Hide resolved
| function buildAttributeString(attributes: Record<string, string>): string { | ||
| const entries = Object.entries(attributes); | ||
| if (entries.length === 0) return ""; | ||
| return ( | ||
| " " + entries.map(([key, value]) => `${key}="${value.replace(/\\"/g, """)}"`).join(" ") | ||
| ); | ||
| } |
There was a problem hiding this comment.
Incorrect regex for escaping quotes.
The regex /\\"/g matches a literal backslash followed by a quote, not just a quote character. This means actual quotes in attribute values won't be escaped to ".
🐛 Fix the regex
function buildAttributeString(attributes: Record<string, string>): string {
const entries = Object.entries(attributes);
if (entries.length === 0) return "";
return (
- " " + entries.map(([key, value]) => `${key}="${value.replace(/\\"/g, """)}"`).join(" ")
+ " " + entries.map(([key, value]) => `${key}="${value.replace(/"/g, """)}"`).join(" ")
);
}🤖 Prompt for AI Agents
In `@apps/pipeline/src/tools/new-tooling/section-wrapper.ts` around lines 52 - 58,
In buildAttributeString, the current regex /\\"/g only matches a backslash+quote
sequence so plain double quotes in attribute values aren't escaped; update the
replacement to use /"/g (i.e., value.replace(/"/g, """)) so all
double-quote characters are converted to " when building the attribute
string.
| const { charactersData, bookData } = useBookConvex(); | ||
| const chapterTitle = useMemo( | ||
| () => bookData!.chapters.find((c) => c.id === String(chapter))?.title, | ||
| [chapter, bookData], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the CharacterModal.tsx file
fd "CharacterModal.tsx" apps/playerRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 127
🏁 Script executed:
# Get the size of the file first
wc -l apps/player/src/components/modals/CharacterModal.tsxRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 131
🏁 Script executed:
# Read the relevant sections around the flagged lines
cat -n apps/player/src/components/modals/CharacterModal.tsx | sed -n '1,70p'Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3372
🏁 Script executed:
# Also read the lines around 344-345 to see usage
cat -n apps/player/src/components/modals/CharacterModal.tsx | sed -n '340,350p'Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 835
🏁 Script executed:
# Find and examine the useBookConvex hook implementation
fd "useBookConvex" apps/player --type fRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 57
🏁 Script executed:
# Check the BookConvexContext implementation
fd "BookConvexContext" apps/player --type fRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 120
🏁 Script executed:
# Read the BookConvexContext implementation
cat -n apps/player/src/context/BookConvexContext.tsxRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 38483
Guard bookData before accessing to prevent crashes during initial Convex data load.
bookData is typed as BookData | null in the context and initialized as null. During the initial load phase, accessing bookData!.chapters will throw a runtime error. Use optional chaining with a safe fallback instead:
🛡️ Safer access
- const chapterTitle = useMemo(
- () => bookData!.chapters.find((c) => c.id === String(chapter))?.title,
- [chapter, bookData],
- );
+ const chapterTitle = useMemo(
+ () =>
+ bookData?.chapters.find((c) => c.id === String(chapter ?? ""))?.title ?? "",
+ [chapter, bookData],
+ );This prevents the error from propagating to the rendered label at line 344-345.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { charactersData, bookData } = useBookConvex(); | |
| const chapterTitle = useMemo( | |
| () => bookData!.chapters.find((c) => c.id === String(chapter))?.title, | |
| [chapter, bookData], | |
| ); | |
| const { charactersData, bookData } = useBookConvex(); | |
| const chapterTitle = useMemo( | |
| () => | |
| bookData?.chapters.find((c) => c.id === String(chapter ?? ""))?.title ?? "", | |
| [chapter, bookData], | |
| ); |
🤖 Prompt for AI Agents
In `@apps/player/src/components/modals/CharacterModal.tsx` around lines 54 - 58,
The computed chapterTitle uses bookData! and can throw during initial load;
update the useMemo in CharacterModal (symbols: useBookConvex, bookData,
chapterTitle) to guard bookData before accessing chapters by using optional
chaining and a safe fallback (e.g., null or an empty string) so chapterTitle is
computed only when bookData is present and never dereferences null during
render.
| // Handle unknown characters (speakers not in characterMetadata) | ||
| if (!matchingCharacter) { | ||
| const displayName = slugToDisplayName(characterSlug); | ||
| // Use generic avatar from Convex if available, otherwise SVG fallback | ||
| const unknownAvatarSrc = | ||
| genericCharacter?.media?.avatarUrl ?? | ||
| getAvatarSource({ | ||
| slug: characterSlug, | ||
| characterName: displayName, | ||
| bookSlug: "", | ||
| infoPerChapter: [], | ||
| }); | ||
|
|
||
| return ( | ||
| <ModalUI onClose={handleOnClose} className="bg-transparent" size="md"> | ||
| <motion.div | ||
| className="flex flex-col items-center gap-4 p-6 bg-black/70 textured-bg border border-white/30 rounded-xl" | ||
| variants={variants.container} | ||
| initial="hidden" | ||
| animate="visible" | ||
| exit="exit" | ||
| onPointerUp={handleOnClose} | ||
| > | ||
| <div className="w-24 h-24 rounded-full overflow-hidden border border-white/20"> | ||
| <img src={unknownAvatarSrc} alt={displayName} className="w-full h-full object-cover" /> | ||
| </div> | ||
| <h4 className="text-lg font-bold text-white">{displayName}</h4> | ||
| <DialogEnhanceClose | ||
| className="absolute top-4 right-4 cursor-pointer" | ||
| onPointerUp={(e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| handleOnClose(); | ||
| }} | ||
| /> | ||
| </motion.div> | ||
| </ModalUI> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/player && find . -name "CharacterModal.tsx" -type fRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 117
🏁 Script executed:
wc -l apps/player/src/components/modals/CharacterModal.tsxRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 131
🏁 Script executed:
sed -n '180,230p' apps/player/src/components/modals/CharacterModal.tsxRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1862
🏁 Script executed:
rg "DialogEnhanceClose" apps/player/src/components/modals/CharacterModal.tsx -B 5 -A 10Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 2179
🏁 Script executed:
fd "DialogEnhanceClose" apps/player/src --type fRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 57
🏁 Script executed:
cat -n apps/player/src/components/ui/dialog.tsx | head -100Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 4369
🏁 Script executed:
rg "export.*DialogEnhanceClose" apps/player/src -B 5 -A 20Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 57
🏁 Script executed:
rg "onKeyDown" apps/player/src/components/modals/CharacterModal.tsx -B 5 -A 5Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 941
🏁 Script executed:
cat apps/player/AGENTS.md | head -150Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1900
🏁 Script executed:
rg "ModalUI" apps/player/src/components -t tsx -A 10 -B 5 | head -80Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 102
🏁 Script executed:
fd "ModalUI\|modal" apps/player/src/components -type f | head -10Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 245
🏁 Script executed:
rg "ModalUI" apps/player/src -A 8 -B 2 | head -120Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 10073
🏁 Script executed:
fd "ModalUI" apps/player/srcRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 120
🏁 Script executed:
rg "function ModalUI|const ModalUI|export.*ModalUI" apps/player/src -A 15Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 2416
🏁 Script executed:
cat apps/player/src/components/modals/ModalUI.tsxRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 18886
🏁 Script executed:
sed -n '1,230p' apps/player/src/components/modals/CharacterModal.tsx | tail -50Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1847
Add keyboard close handling for the unknown-character modal.
The close control at line 215 only handles pointer events, while the main character modal's close button also supports Enter/Space. This inconsistency within the same file should be aligned.
♿ Keyboard handler parity
<DialogEnhanceClose
className="absolute top-4 right-4 cursor-pointer"
onPointerUp={(e) => {
e.preventDefault();
e.stopPropagation();
handleOnClose();
}}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ handleOnClose();
+ }
+ }}
/>🤖 Prompt for AI Agents
In `@apps/player/src/components/modals/CharacterModal.tsx` around lines 188 - 226,
The unknown-character modal's close control (DialogEnhanceClose) only handles
pointer events—add keyboard support to match the main modal by handling
onKeyDown for Enter/Space and ensuring it calls handleOnClose; update the
DialogEnhanceClose element in the unknown-character branch to include an
onKeyDown handler that prevents default/propagation for Enter (key === "Enter")
and Space (key === " " or key === "Spacebar") and then invokes handleOnClose,
and ensure the element is focusable (e.g., has tabIndex={0} and appropriate
aria-label) so keyboard users can reach it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/pipeline/src/scripts/fix-unwrapped-paragraphs-in-temporary-output.ts`:
- Around line 19-33: parseArgs currently passes undefined to resolvePath when a
flag like --source or --out is present but has no following value; update
parseArgs to validate that when sourceIdx or outputIdx !== -1 the next token
(args[sourceIdx+1] / args[outputIdx+1]) exists and is not another flag (e.g.
startsWith("--")), and if invalid throw a clear Error or log a message and exit
(include which flag is missing a value), before calling resolvePath; keep
existing behavior for slugsIdx but ensure you reference parseArgs, sourceIdx,
outputIdx, slugsIdx and DEFAULT_OUTPUT_ROOT in your checks.
🧹 Nitpick comments (1)
apps/pipeline/src/scripts/fix-unwrapped-paragraphs-in-temporary-output.ts (1)
7-8: Prefer a portable temp directory default.
The hard-coded macOS path makes the script fragile across environments. Consider deriving from the OS temp directory.♻️ Proposed change
+import os from "os"; import fs from "fs"; import path from "path"; @@ -const DEFAULT_OUTPUT_ROOT = - "/var/folders/j9/pbqwg7zs4336w7vccnz2xhcw0000gn/T/bookgenius-fixed-unwrapped"; +const DEFAULT_OUTPUT_ROOT = path.join(os.tmpdir(), "bookgenius-fixed-unwrapped");
| function parseArgs(): Args { | ||
| const args = process.argv.slice(2); | ||
| const sourceIdx = args.indexOf("--source"); | ||
| const outputIdx = args.indexOf("--out"); | ||
| const slugsIdx = args.indexOf("--slugs"); | ||
|
|
||
| const repoRoot = path.resolve(process.cwd()); | ||
| const defaultSource = path.join(repoRoot, "apps", "pipeline", "books-data"); | ||
|
|
||
| const sourceRoot = resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource); | ||
| const outputRoot = resolvePath(outputIdx !== -1 ? args[outputIdx + 1] : DEFAULT_OUTPUT_ROOT); | ||
|
|
||
| let slugs: string[] = []; | ||
| if (slugsIdx !== -1) { | ||
| slugs = args[slugsIdx + 1]?.split(",").map((slug) => slug.trim()) ?? []; |
There was a problem hiding this comment.
Guard against missing CLI flag values.
If --source or --out is provided without a value, resolvePath receives undefined. Consider explicit validation with a clear error.
🛠️ Suggested guard
const sourceIdx = args.indexOf("--source");
const outputIdx = args.indexOf("--out");
@@
- const sourceRoot = resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource);
- const outputRoot = resolvePath(outputIdx !== -1 ? args[outputIdx + 1] : DEFAULT_OUTPUT_ROOT);
+ if (sourceIdx !== -1 && !args[sourceIdx + 1]) {
+ throw new Error("Missing value for --source");
+ }
+ if (outputIdx !== -1 && !args[outputIdx + 1]) {
+ throw new Error("Missing value for --out");
+ }
+ const sourceRoot = resolvePath(sourceIdx !== -1 ? args[sourceIdx + 1] : defaultSource);
+ const outputRoot = resolvePath(outputIdx !== -1 ? args[outputIdx + 1] : DEFAULT_OUTPUT_ROOT);🤖 Prompt for AI Agents
In `@apps/pipeline/src/scripts/fix-unwrapped-paragraphs-in-temporary-output.ts`
around lines 19 - 33, parseArgs currently passes undefined to resolvePath when a
flag like --source or --out is present but has no following value; update
parseArgs to validate that when sourceIdx or outputIdx !== -1 the next token
(args[sourceIdx+1] / args[outputIdx+1]) exists and is not another flag (e.g.
startsWith("--")), and if invalid throw a clear Error or log a message and exit
(include which flag is missing a value), before calling resolvePath; keep
existing behavior for slugsIdx but ensure you reference parseArgs, sourceIdx,
outputIdx, slugsIdx and DEFAULT_OUTPUT_ROOT in your checks.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/player/src/ui/activateMediaInRange.ts (1)
734-743:⚠️ Potential issue | 🟠 MajorAdd fallback handling for unknown characters in drama tables.
The persona cell hydration (drama table cells) returns early when
characterDatais undefined (line 742), while the inline avatar code above it explicitly handles unknown characters with a generic avatar fallback. SincegenericCharacteris available in this scope and the codebase already has fallback handling patterns for unknown characters, drama tables should use similar fallback logic instead of returning early.Related code patterns
The inline avatar code (lines 729-731) passes
genericCharacteras a fallback:populateInlineAvatarShell(shell, characterData, location, undefined, genericCharacter);The
populateInlineAvatarShellfunction (lines 329-340) handles undefinedcharacterData:if (!characterData) { const fallbackSrc = genericAvatarUrl ?? generateFallbackAvatarUrl(characterSlug);apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts (1)
230-244:⚠️ Potential issue | 🟡 MinorSilent failure may cause missing chapters without indication.
If both
callGrokAzureWithSchemaand the Gemini fallback fail, the function returns early (line 243) without adding an entry tojsonSummaries. The final output will silently omit failed chapters with no aggregated error report.Consider tracking failures and logging a summary at the end, or failing the entire batch if critical chapters are missing.
🛠️ Suggested approach
+ const failedChapters: number[] = []; // ... in the catch block at line 241-243: } catch (e) { console.error(`Error for chapter ${chapterNum}`, e); + failedChapters.push(chapterNum); return; } // ... after Promise.all: + if (failedChapters.length > 0) { + console.warn(`Failed to generate summaries for chapters: ${failedChapters.join(", ")}`); + }apps/pipeline/src/tools/new-tooling/generate-prompts-for-backgrounds.ts (1)
159-186:⚠️ Potential issue | 🟠 MajorChunk chapter content before the Gemini call to respect LLM context limits.
Line 177 injects the full
chapter.contentinto the prompt without length limiting or chunking. This can exceed LLM context windows and degrade quality/reliability. Apply the same approach used increate-graphical-style.ts(accumulate text with length check:if (bookText.length > 10000) break;) or split content into chunks and use the first one, consistent with the coding guideline: Split long chapters into chunks when processing with LLM to respect context limits (apps/pipeline/src/tools/**/*.ts).Code location
Chapter Text: <chapter>${chapter.content}</chapter>apps/pipeline/src/tools/new-tooling/generate-flux-schnel-image.ts (1)
225-235:⚠️ Potential issue | 🟠 MajorReplace explicit test prompt with appropriate content.
The test prompt contains explicit sexual content (
"naked, breasts exposed, seductive pose, sexy"). Even in test/debug code, this is problematic:
- Committed to version control and visible to all contributors
- May violate Replicate's terms of service
- Unprofessional in a production codebase
Replace with a neutral prompt that still tests the avatar generation flow.
🛡️ Proposed fix with neutral test prompt
if (require.main === module) { generateFluxImage( - "A beautiful woman with long brown hair and blue eyes, seductive pose, naked, red lips, sexy, elegant, beautiful, 18 years old, breasts exposed", + "A portrait of a person with long brown hair and blue eyes, elegant pose, wearing formal attire, professional lighting", "test", "SinCity style", "avatar", "test.webp", ).then((image) => { console.log(`image: ${image}`); }); }apps/pipeline/src/tools/fixLongXml.ts (1)
61-87:⚠️ Potential issue | 🟠 MajorHTML parsing with
"text/html"wraps output in<html><body>tags, bypassing single-<Chapter>detection.With
"text/html",@xmldomcreates an HTML document where content is wrapped inside<body>. The single-chapter check at line 74 fails becausedoc.documentElementis<html>, not<Chapter>, so the code falls through to document-wide serialization that includes HTML wrapper tags.Consider using
"text/xml"instead (matching patterns increate-book-metadata.tsandpipeline-ui/src/utils/xmlParser.ts), or extractbodychildren explicitly without altering tag name case sensitivity:- const doc = parser.parseFromString(xml, "text/html"); + const doc = parser.parseFromString(xml, "text/xml");Alternatively, if HTML parsing is intentional, extract body contents:
+ const body = doc.getElementsByTagName("body")[0]; + const contentRoot = body ?? doc.documentElement; - const root = doc.documentElement as Element | null; - if (root && root.tagName === "Chapter") { + const root = body?.firstElementChild ?? doc.documentElement; + if (root && root.tagName === "Chapter") { return formatChapterElement(root, serializer) + "\n"; } - let whole = serializer.serializeToString(doc); + let whole = body + ? Array.from(body.childNodes).map((n) => serializer.serializeToString(n)).join("") + : serializer.serializeToString(doc);apps/pipeline/src/server/pipeline.ts (1)
694-695:⚠️ Potential issue | 🟠 MajorHardcoded
isStandardEbook = truebypasses detection logic.Line 695 hardcodes
isStandardEbook = true, making the condition check on lines 691-693 (checking forseMetadataPathandseCoverPath) ineffective. This appears to be debug code that was left in.🐛 Proposed fix
const seMetadataPath = path.join(seBookDir, "metadata.json"); const seCoverPath = path.join(seBookDir, "images", "cover.jpg"); - // const isStandardEbook = fs.existsSync(seMetadataPath) && fs.existsSync(seCoverPath); - const isStandardEbook = true; + const isStandardEbook = fs.existsSync(seMetadataPath) && fs.existsSync(seCoverPath);
🤖 Fix all issues with AI agents
In `@apps/pipeline/src/callFastGemini.ts`:
- Around line 78-94: The function callGeminiWithThinking is missing the
thinkingConfig and uses an inconsistent HarmBlockThreshold; update the
providerOptions passed to generateText inside callGeminiWithThinking to include
thinkingConfig: { thinkingBudget: 256, includeThoughts: true } (matching
callFastGemini) and change all safetySettings thresholds from
HarmBlockThreshold.OFF to HarmBlockThreshold.BLOCK_NONE so behavior matches
other Gemini variants; ensure you only modify the providerOptions block in
callGeminiWithThinking and keep the rest of the call (model, prompt,
experimental_telemetry) unchanged.
In `@apps/pipeline/src/callO3.ts`:
- Around line 36-39: The callGpt5 function currently calls
client.chat.completions.create (chatCompletion) but does not implement the
callModel<T> interface: it accepts schema and maxRetries but never uses them;
update callGpt5 to either (A) implement structured output handling by using the
provided schema to request and validate structured output (e.g., include a
system/instruction to output JSON matching schema, parse the model response into
type T and validate against schema) and add retry logic around
client.chat.completions.create honoring the maxRetries parameter with
exponential backoff and proper error propagation, or (B) remove the unused
schema and maxRetries parameters and update the signature to match the intended
simple call; reference symbols: callGpt5, callModel<T>, schema, maxRetries, and
client.chat.completions.create (chatCompletion) so reviewers can locate and
verify the changes.
In `@apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.ts`:
- Line 20: The JSDOM instance in wrapChaptersWithSections.ts is created with
contentType: "text/html", causing the FB2 XML namespace to be lost; change the
JSDOM constructor call that creates dom (the line with const dom = new
JSDOM(xml, { contentType: ... })) to use contentType: "text/xml" so the original
FB2 namespace and namespaceURIs are preserved and newly created elements via
createElementNS(...) won't get spurious xmlns="" attributes.
In `@apps/pipeline/src/tools/new-tooling/generate-pictures-for-entities.ts`:
- Around line 294-298: Prevent duplicate "generic-avatar" entries by checking
response.characters for an existing item with name === "generic-avatar" before
calling response.characters.push; locate the push call
(response.characters.push({...}) in generate-pictures-for-entities.ts) and guard
it with a conditional that searches the response.characters array (e.g., using
.some or .find) and only pushes the new object when no matching name exists, so
you avoid redundant entries and duplicate generation/overwrites.
In
`@apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts`:
- Line 231: Fix the typo in the prompt string(s) inside the
get-chapter-by-chapter-with-paragraphs-json-summary code: change "buts its VERY
important" to "but it's VERY important" in the template literal that includes
`${prompt}` (update both occurrences of this phrase found in the prompt
construction).
- Around line 167-169: The current flow calls getParagraphsFromChapter(...) and
then buildParagraphsForSummary(...) without splitting large chapters; update
this to detect large inputs using needsChunking(paragraphsFromChapter) and, if
true, call chunkParagraphs(paragraphsFromChapter) and then process each chunk
through buildParagraphsForSummary (or otherwise aggregate per-chunk summaries)
so the downstream LLM calls handle chunked inputs; follow the same pattern used
in identifyEntityAndRewriteParagraphs (use needsChunking and chunkParagraphs
utilities) to locate where to branch and ensure paragraphsForPage is built from
chunked results when required.
In `@apps/pipeline/src/tools/new-tooling/restore-unwrapped-lines.ts`:
- Around line 68-79: The loop in restore-unwrapped-lines.ts currently skips any
line where trimmed.startsWith("<"), which incorrectly skips lines beginning with
inline tags like <span> or <em>; change that check to only skip block-level or
special tags by replacing the trimmed.startsWith("<") test with a more specific
predicate (e.g., a regex or helper like isBlockTag(trimmed)) that matches only
block-level/opening/closing tags and comments (for example:
/^<\/?(?:p|div|h[1-6]|ul|ol|li|pre|code|blockquote|table|thead|tbody|tr|td|th|section|article|header|footer|aside)\b/i
or /^<!--/), leaving lines that start with inline tags to be processed normally
by the remainder of the function (the loop over lines with variables lines,
trimmed, output).
In `@apps/pipeline/src/tools/new-tooling/sanitize-nested-paragraphs.ts`:
- Around line 20-41: The checks using lower.startsWith("<p") and
lower.startsWith("</p") are too broad and match tags like <pre> or <picture>;
update the conditional logic around the tag/lower variables in
sanitize-nested-paragraphs.ts so the opening check only matches an actual <p>
tag (e.g. require "<p" followed immediately by whitespace or '>' using a regex
like /^<p(?=\s|>)/i) and the closing check only matches a real </p> (e.g.
/^<\/p(?=\s|>)/i); apply these stricter matches where tag/lower are used to
decide nesting (both the opening-branch that currently uses
lower.startsWith("<p") and the closing-branch that uses lower.startsWith("</p"))
so <pre>, <picture>, etc. are not treated as paragraph tags.
In `@apps/player/src/ui/activateMediaInRange.ts`:
- Line 715: Remove the stray debug console.log in activateMediaInRange.ts that
prints getCharactersData(); locate the console.log("getCharactersData",
getCharactersData()) call (inside the activateMediaInRange flow) and delete it
or replace it with the appropriate production logger if needed (e.g., use
existing processLogger or debug util) so no console.debug remains in the merged
code.
- Around line 329-360: Remove the two debug console.log calls inside the
unknown-character branch of populateInlineAvatarShell (the lines logging
"generic" and the `[populateInlineAvatarShell] Unknown character` object);
either delete them or replace them with a conditional/debug-level logger if an
application debug logger exists, ensuring no verbose console output remains for
genericCharacter/genericAvatarUrl/genericMedia in production while leaving the
rest of the fallback avatar construction (generateFallbackAvatarUrl,
normalizeSrcForInlineAvatar, shell.title assignment, and placeholderImg
creation) unchanged.
In `@convex/paragraphEditor.ts`:
- Around line 243-245: The current flow parses htmlResult.content with new
DOMParser().parseFromString(..., "text/html") and then serializes the entire
document, which preserves unwanted <html><head><body> wrappers; update each
place where parser and doc are used (where you call parseFromString on
htmlResult.content) to extract and serialize only the document body—e.g., obtain
doc.body and produce the stored HTML from doc.body (via innerHTML or an
XMLSerializer on doc.body) instead of serializing the whole document—apply this
change to all four handlers that use parser/doc to ensure only body content is
persisted.
🧹 Nitpick comments (7)
apps/pipeline/src/callClaude.ts (1)
188-193: Test prompt appears incomplete.The prompt references "this page" but no page content is provided. This will likely produce unhelpful results when running the module directly for testing. Consider providing sample input or clarifying if this is intentional placeholder code.
const doIt = async () => { + const samplePage = "John walked into the room where Mary was waiting..."; // Add test content const result = (await callGeminiWrapper( - "Identify all named book characters (people) in this page.\n", + `Identify all named book characters (people) in this page:\n${samplePage}`, undefined, 1, )) as string; logger.info(result); };apps/pipeline-ui/src/pages/StandardEbooksPage.tsx (1)
184-210: Consider memoizing the filtering and grouping logic.This computation runs on every render. For a large book collection, wrapping it in
useMemowould prevent unnecessary recalculations when unrelated state (likemodalBook) changes.♻️ Suggested refactor with useMemo
+ const { filteredBooks, visibleGroupedBooks, sortedLetters, visibleCount } = useMemo(() => { + const normalizedQuery = searchQuery.trim().toLowerCase(); + const filtered = normalizedQuery + ? allBooks.filter((book) => { + const haystack = [ + book.title, + book.author, + book.authorFileAs, + book.description, + book.subjects.join(" "), + ] + .join(" ") + .toLowerCase(); + return haystack.includes(normalizedQuery); + }) + : allBooks; + + const grouped = normalizedQuery + ? filtered.reduce<Record<string, SEBook[]>>((acc, book) => { + const firstLetter = (book.authorFileAs || book.author).charAt(0).toUpperCase(); + if (!acc[firstLetter]) acc[firstLetter] = []; + acc[firstLetter].push(book); + return acc; + }, {}) + : groupedBooks; + + return { + filteredBooks: filtered, + visibleGroupedBooks: grouped, + sortedLetters: Object.keys(grouped).sort(), + visibleCount: normalizedQuery ? filtered.length : totalBooks, + }; + }, [searchQuery, allBooks, groupedBooks, totalBooks]); - const normalizedQuery = searchQuery.trim().toLowerCase(); - const filteredBooks = normalizedQuery - ? allBooks.filter((book) => { - const haystack = [ - book.title, - book.author, - book.authorFileAs, - book.description, - book.subjects.join(" "), - ] - .join(" ") - .toLowerCase(); - return haystack.includes(normalizedQuery); - }) - : allBooks; - - const visibleGroupedBooks = normalizedQuery - ? filteredBooks.reduce<Record<string, SEBook[]>>((acc, book) => { - const firstLetter = (book.authorFileAs || book.author).charAt(0).toUpperCase(); - if (!acc[firstLetter]) acc[firstLetter] = []; - acc[firstLetter].push(book); - return acc; - }, {}) - : groupedBooks; - - const sortedLetters = Object.keys(visibleGroupedBooks).sort(); - const visibleCount = normalizedQuery ? filteredBooks.length : totalBooks;Note: You'll also need to derive
normalizedQueryseparately for the conditional display logic at lines 253-255, or refactor that condition to checksearchQuery.trim()directly.apps/pipeline/src/callFastGemini.ts (1)
86-86: Consider removing or leveling the debug log.The
console.log("CALLING GEMINI WITH THINKING")statement may produce noise in production logs. If logging is needed, consider using a structured logger with appropriate log levels.apps/pipeline/src/tools/new-tooling/generate-pictures-for-entities.ts (1)
311-319: Prefer structured logger overconsole.logfor cache diagnostics.
Keeps logs consistent and easier to query across pipeline runs.Proposed refactor
- console.log("[generatePicturesForEntities] Using cached generated-prompts.json"); - console.log( - "[generatePicturesForEntities] Cached characters:", - generatedPrompts.characters.map((c) => c.name), - ); - console.log( - "[generatePicturesForEntities] Reference cards characters:", - referenceCards.characters.map((c) => c.name), - ); + logger.info("[generatePicturesForEntities] Using cached generated-prompts.json"); + logger.info( + "[generatePicturesForEntities] Cached characters:", + generatedPrompts.characters.map((c) => c.name), + ); + logger.info( + "[generatePicturesForEntities] Reference cards characters:", + referenceCards.characters.map((c) => c.name), + );apps/pipeline/src/tools/new-tooling/generate-flux-schnel-image.ts (1)
142-152: Consider removing commented-out code.This commented block appears to serve as documentation for the old flux-2-pro input format. If this historical reference is needed, consider moving it to a separate documentation file or commit message rather than keeping dead code in the source.
apps/pipeline/src/tools/identifyEntityAndRewriteParagraphs.ts (1)
156-176: Extract the restore/sanitize pipeline to a helper to avoid drift.The same restoration chain is duplicated in chunked and single-shot flows.
♻️ Suggested refactor
+function restoreAndSanitizeXml( + originalXml: string, + modelXml: string, + contextLabel: string, +): string { + let restored = modelXml; + try { + restored = restoreOriginalTextInHtml(originalXml, restored); + } catch (e) { + logger.error(`Error restoring original text for ${contextLabel}`, e); + } + try { + restored = restoreUnwrappedLines(originalXml, restored); + } catch (e) { + logger.error(`Error restoring unwrapped lines for ${contextLabel}`, e); + } + try { + restored = sanitizeNestedParagraphs(restored); + } catch (e) { + logger.error(`Error sanitizing nested paragraphs for ${contextLabel}`, e); + } + return restored; +}- let restored = clearedResponse; - try { - restored = restoreOriginalTextInHtml(originalChunkXml, clearedResponse); - } catch (e) { - logger.error(`Error restoring original text for chapter ${chapter} chunk ${chunkIndex}`, e); - } - - try { - restored = restoreUnwrappedLines(originalChunkXml, restored); - } catch (e) { - logger.error(`Error restoring unwrapped lines for chapter ${chapter} chunk ${chunkIndex}`, e); - } - - try { - restored = sanitizeNestedParagraphs(restored); - } catch (e) { - logger.error( - `Error sanitizing nested paragraphs for chapter ${chapter} chunk ${chunkIndex}`, - e, - ); - } + const restored = restoreAndSanitizeXml( + originalChunkXml, + clearedResponse, + `chapter ${chapter} chunk ${chunkIndex}`, + );- let restored = clearedResponse; - try { - restored = restoreOriginalTextInHtml(paragraphsForPage, clearedResponse); - } catch (e) { - logger.error("Error restoring original text for chapter " + chapter, e); - } - - try { - restored = restoreUnwrappedLines(paragraphsForPage, restored); - } catch (e) { - logger.error("Error restoring unwrapped lines for chapter " + chapter, e); - } - - try { - restored = sanitizeNestedParagraphs(restored); - } catch (e) { - logger.error("Error sanitizing nested paragraphs for chapter " + chapter, e); - } + const restored = restoreAndSanitizeXml( + paragraphsForPage, + clearedResponse, + `chapter ${chapter}`, + );apps/pipeline/src/server/upload-figures-cli.ts (1)
13-24: Consider extractinggetContentTypeto a shared utility.This function duplicates logic from
pipeline.ts(lines 129-145), which has a more complete set of MIME type mappings (including.mp4,.webm,.mp3,.xml,.json). Consider extracting to a shared helper to avoid drift between implementations.♻️ Suggested shared utility
Create a shared helper, e.g.,
apps/pipeline/src/lib/contentType.ts:export function getContentType(filename: string): string { const ext = path.extname(filename).toLowerCase(); const types: Record<string, string> = { ".png": "image/png", ".jpg": "image/jpeg", ".jpeg": "image/jpeg", ".gif": "image/gif", ".svg": "image/svg+xml", ".webp": "image/webp", ".mp4": "video/mp4", ".webm": "video/webm", ".mp3": "audio/mpeg", ".xml": "application/xml", ".json": "application/json", }; return types[ext] || "application/octet-stream"; }Then import it in both
pipeline.tsandupload-figures-cli.ts.
| export const callGeminiWithThinking = async (prompt: string) => { | ||
| const ai = new GoogleGenAI({ apiKey: process.env.GOOGLE_GENERATIVE_AI_API_KEY }); | ||
| const config = { | ||
| responseMimeType: "text/plain", | ||
| httpOptions: { | ||
| timeout: 15 * 60 * 1000, // 15 minutes in milliseconds | ||
| }, | ||
| }; | ||
| const model = "gemini-3-flash-preview"; | ||
| // const model = "gemini-3-pro-preview"; | ||
|
|
||
| const contents = [{ role: "user", parts: [{ text: prompt }] }]; | ||
| const safetySettings = [ | ||
| { category: HarmCategory.HARM_CATEGORY_HARASSMENT, threshold: HarmBlockThreshold.BLOCK_NONE }, | ||
| { category: HarmCategory.HARM_CATEGORY_HATE_SPEECH, threshold: HarmBlockThreshold.BLOCK_NONE }, | ||
| { | ||
| category: HarmCategory.HARM_CATEGORY_SEXUALLY_EXPLICIT, | ||
| threshold: HarmBlockThreshold.BLOCK_NONE, | ||
| }, | ||
| { | ||
| category: HarmCategory.HARM_CATEGORY_DANGEROUS_CONTENT, | ||
| threshold: HarmBlockThreshold.BLOCK_NONE, | ||
| }, | ||
| { | ||
| category: HarmCategory.HARM_CATEGORY_CIVIC_INTEGRITY, | ||
| threshold: HarmBlockThreshold.BLOCK_NONE, | ||
| }, | ||
| { category: HarmCategory.HARM_CATEGORY_HARASSMENT, threshold: HarmBlockThreshold.OFF }, | ||
| { category: HarmCategory.HARM_CATEGORY_HATE_SPEECH, threshold: HarmBlockThreshold.OFF }, | ||
| { category: HarmCategory.HARM_CATEGORY_SEXUALLY_EXPLICIT, threshold: HarmBlockThreshold.OFF }, | ||
| { category: HarmCategory.HARM_CATEGORY_DANGEROUS_CONTENT, threshold: HarmBlockThreshold.OFF }, | ||
| { category: HarmCategory.HARM_CATEGORY_CIVIC_INTEGRITY, threshold: HarmBlockThreshold.OFF }, | ||
| ]; | ||
|
|
||
| console.log("before response", model); | ||
| const response = await ai.models.generateContent({ | ||
| model, | ||
| config: { ...config, safetySettings }, | ||
| contents, | ||
| console.log("CALLING GEMINI WITH THINKING"); | ||
| const { text } = await generateText({ | ||
| model: google("gemini-3-flash-preview"), | ||
| prompt, | ||
| experimental_telemetry: { isEnabled: true, recordInputs: true, recordOutputs: true }, | ||
| providerOptions: { google: { safetySettings } }, | ||
| }); | ||
|
|
||
| console.log("after response"); | ||
|
|
||
| return response?.text; | ||
| return text; | ||
| }; |
There was a problem hiding this comment.
Missing thinkingConfig despite function name implying thinking is enabled.
The function callGeminiWithThinking no longer configures thinking in providerOptions. Compare with callFastGemini (lines 20-25) which includes thinkingConfig: { thinkingBudget: 256, includeThoughts: true }. The function name is now misleading—either add the thinking configuration or rename the function.
Additionally, this function uses HarmBlockThreshold.OFF while all other functions in this file use HarmBlockThreshold.BLOCK_NONE. This inconsistency may cause unexpected behavior differences between Gemini call variants.
Proposed fix to add thinking configuration
const { text } = await generateText({
model: google("gemini-3-flash-preview"),
prompt,
experimental_telemetry: { isEnabled: true, recordInputs: true, recordOutputs: true },
- providerOptions: { google: { safetySettings } },
+ providerOptions: {
+ google: {
+ safetySettings,
+ thinkingConfig: { thinkingBudget: 1024, includeThoughts: true },
+ },
+ },
});🤖 Prompt for AI Agents
In `@apps/pipeline/src/callFastGemini.ts` around lines 78 - 94, The function
callGeminiWithThinking is missing the thinkingConfig and uses an inconsistent
HarmBlockThreshold; update the providerOptions passed to generateText inside
callGeminiWithThinking to include thinkingConfig: { thinkingBudget: 256,
includeThoughts: true } (matching callFastGemini) and change all safetySettings
thresholds from HarmBlockThreshold.OFF to HarmBlockThreshold.BLOCK_NONE so
behavior matches other Gemini variants; ensure you only modify the
providerOptions block in callGeminiWithThinking and keep the rest of the call
(model, prompt, experimental_telemetry) unchanged.
| const chatCompletion = await client.chat.completions.create({ | ||
| messages: [{ role: "user", content: prompt }], | ||
| model: "gpt-5.1", | ||
| model: "gpt-5.2", | ||
| reasoning_effort: "medium", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OpenAI gpt-5.2 model availability chat completions API 2026
💡 Result:
As of February 5, 2026, GPT‑5.2 is available to all developers in the OpenAI API, including the Chat Completions API.
- Chat Completions API model IDs
gpt-5.2(GPT‑5.2 Thinking) — usable in Chat Completions (/v1/chat/completions) and also in Responses. (openai.com)gpt-5.2-chat-latest(GPT‑5.2 Instant) — intended for chat-style usage. (openai.com)
- Pinned snapshot available:
gpt-5.2-2025-12-11(plus aliasgpt-5.2). (platform.openai.com) - Docs / pricing / capabilities page: confirms modalities (text I/O, image input) and that it supports Chat Completions. (platform.openai.com)
Citations:
- 1: https://openai.com/index/introducing-gpt-5-2//?utm_source=openai
- 2: https://openai.com/index/introducing-gpt-5-2//?utm_source=openai
- 3: https://platform.openai.com/docs/models/gpt-5.2/?utm_source=openai
- 4: https://platform.openai.com/docs/models/gpt-5.2/?utm_source=openai
🏁 Script executed:
cat -n apps/pipeline/src/callO3.tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1671
Remove the review concern: gpt-5.2 is available in the Chat Completions API.
The model is officially supported for all developers as of February 2026, including the direct OpenAI SDK path used here.
However, the callGpt5 function does not implement the required callModel<T> interface specified in the coding guidelines for apps/pipeline/src/call*.ts. The function accepts schema and maxRetries parameters but does not use them. Implement structured output handling via the schema parameter and retry logic to comply with the shared interface, or remove the unused parameters.
🤖 Prompt for AI Agents
In `@apps/pipeline/src/callO3.ts` around lines 36 - 39, The callGpt5 function
currently calls client.chat.completions.create (chatCompletion) but does not
implement the callModel<T> interface: it accepts schema and maxRetries but never
uses them; update callGpt5 to either (A) implement structured output handling by
using the provided schema to request and validate structured output (e.g.,
include a system/instruction to output JSON matching schema, parse the model
response into type T and validate against schema) and add retry logic around
client.chat.completions.create honoring the maxRetries parameter with
exponential backoff and proper error propagation, or (B) remove the unused
schema and maxRetries parameters and update the signature to match the intended
simple call; reference symbols: callGpt5, callModel<T>, schema, maxRetries, and
client.chat.completions.create (chatCompletion) so reviewers can locate and
verify the changes.
| */ | ||
| export function wrapChaptersWithSections(xml: string): string { | ||
| const dom = new JSDOM(xml, { contentType: "text/xml" }); | ||
| const dom = new JSDOM(xml, { contentType: "text/html" }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and the specific file
find apps/pipeline/src/tools -type f -name "wrapChaptersWithSections.ts" | head -5Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 144
🏁 Script executed:
# Read the entire file to understand context and line 20
if [ -f "apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.ts" ]; then
wc -l "apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.ts"
fiRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 147
🏁 Script executed:
# Get the full content with line numbers
cat -n "apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.ts"Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3182
🏁 Script executed:
# Also check if there's documentation about these tools
ls -la apps/pipeline/AGENTS.md 2>/dev/null || echo "AGENTS.md not found"Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 147
🏁 Script executed:
cat -n apps/pipeline/AGENTS.mdRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 3704
🏁 Script executed:
# Check if there are tests for this tool that show input/output
find apps/pipeline -name "*wrapChapters*test*" -o -name "*test*wrapChapters*" 2>/dev/nullRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 57
🏁 Script executed:
# Also search for usage of wrapChaptersWithSections
rg "wrapChaptersWithSections" --type ts -B 2 -A 2Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 2080
🏁 Script executed:
cat -n apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.spec.tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 2750
🏁 Script executed:
# Also check what minifyXml does
rg "minifyXml" apps/pipeline/src/tools/generate-book-cli/ -A 5Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1697
🏁 Script executed:
# Check if there are any test results or if we can see what dom.serialize() would produce
cd apps/pipeline && npm test -- wrapChaptersWithSections.spec.ts 2>&1 | head -100 || echo "Test framework not available in sandbox"Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 479
🏁 Script executed:
# Let's create a simple test to see what JSDOM actually outputs with HTML vs XML parsing
node << 'EOF'
const { JSDOM } = require("jsdom");
const testXml = `<FictionBook xmlns="http://www.gribuser.ru/xml/fictionbook/2.0">
<body>
<section>
<p><strong>Chapter 1</strong></p>
</section>
</body>
</FictionBook>`;
// Test with text/html
const domHtml = new JSDOM(testXml, { contentType: "text/html" });
const docHtml = domHtml.window.document;
console.log("=== HTML parsing ===");
console.log("documentElement.namespaceURI:", docHtml.documentElement.namespaceURI);
console.log("Serialized output (first 500 chars):");
console.log(domHtml.serialize().substring(0, 500));
console.log("\n=== XML parsing ===");
const domXml = new JSDOM(testXml, { contentType: "text/xml" });
const docXml = domXml.window.document;
console.log("documentElement.namespaceURI:", docXml.documentElement.namespaceURI);
console.log("Serialized output (first 500 chars):");
console.log(domXml.serialize().substring(0, 500));
EOFRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 910
🌐 Web query:
JSDOM text/html vs text/xml contentType parsing differences namespaceURI
💡 Result:
In jsdom, contentType controls whether your string is parsed with the HTML parser (text/html) or an XML parser (text/xml, application/xml, application/xhtml+xml, image/svg+xml). If you pick a non‑HTML/non‑XML MIME type, jsdom throws. [1][2]
text/html (HTML parsing)
- Case-insensitive HTML rules apply (tag names are normalized like browsers do). This can change the tree if your input “looks like” HTML (e.g., optional end tags, tag-soup recovery). [3]
- Elements end up in the HTML namespace:
element.namespaceURI === "http://www.w3.org/1999/xhtml"for normal HTML elements (e.g.,div,p).
- Colons don’t create namespaces in HTML parsing: a tag like
<cp:coreProperties>is typically treated as an element whose name contains a colon, not as “prefixcpbound viaxmlns:cp”. SonamespaceURIgenerally won’t reflect yourxmlns:*bindings the way XML does.
text/xml (XML parsing)
- XML is case-sensitive and must be well-formed (single root element, properly nested/closed tags, etc.); otherwise XML parsing can fail/produce a parser error document (browser behavior; jsdom follows the same basic model). [4]
xmlnsdeclarations are honored:- For
<cp:coreProperties xmlns:cp="…">, you get:el.prefix === "cp"el.localName === "coreProperties"el.namespaceURI === "…"(the URI fromxmlns:cp)
This is the DOM’s namespace model: prefix/localName/namespace are derived from the qualified name and namespace bindings. [5]
- For
Practical implication for namespaceURI in jsdom
- If you need correct
namespaceURI/localName/prefixfromxmlns:*, parse as XML:new JSDOM(xmlString, { contentType: "text/xml" })[1][3]- or
new window.DOMParser().parseFromString(xmlString, "text/xml")[4]
- If you parse the same markup as HTML (
text/html), you generally should expect HTML-namespace elements and not real XML namespace binding behavior. [1][4]
Sources: jsdom contentType option docs [1][2], MDN DOMParser/parseFromString MIME-type behavior [4], example of differing behavior when parsing XML as HTML vs forcing text/xml [3], DOM Standard namespace model (prefix/localName/namespace) [5].
🏁 Script executed:
# Let's check if there's any documentation or configuration in the repo about JSDOM usage
rg "JSDOM\|contentType" apps/pipeline --type ts -B 1 -A 1 | head -50Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 57
🏁 Script executed:
# Check if there are any other files that use JSDOM and how they handle it
find apps/pipeline -name "*.ts" -type f -exec grep -l "new JSDOM" {} \;Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1001
🏁 Script executed:
# Check the domParser.ts to see how namespace handling is done elsewhere
cat -n apps/pipeline/src/lib/domParser.tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 569
🏁 Script executed:
# Check how fb2Converter handles JSDOM/parsing
head -50 apps/pipeline/src/tools/fb2-converter/fb2Converter.tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1618
🏁 Script executed:
# Let's see more of fb2Converter to understand how it parses FB2
sed -n '1,150p' apps/pipeline/src/tools/fb2-converter/fb2Converter.ts | tail -100Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3877
🏁 Script executed:
# Check if there are any comments or explanations about why text/html is used in wrapChaptersWithSections
git log --oneline -p apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.ts 2>/dev/null | head -200 || echo "Git history not available"Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3117
🏁 Script executed:
# Check what the actual workflow is - where does wrapChaptersWithSections fit in the pipeline
rg "wrapChaptersWithSections" apps/pipeline --type ts -B 5 -A 5Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3323
Change JSDOM parsing to text/xml to preserve FB2 namespace structure.
At line 20, contentType: "text/html" causes JSDOM to parse with HTML rules, putting documentElement.namespaceURI in the XHTML namespace (http://www.w3.org/1999/xhtml) instead of the original FB2 namespace. This corrupts newly created elements via createElementNS(NS, ...), breaking the docstring's guarantee of "no extra xmlns="" attributes" and producing output with incorrect or missing namespace declarations.
Since the input is always FB2/XML (per the docstring and test), use contentType: "text/xml" to properly preserve and respect the original namespace bindings.
🔧 Fix
- const dom = new JSDOM(xml, { contentType: "text/html" });
+ const dom = new JSDOM(xml, { contentType: "text/xml" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dom = new JSDOM(xml, { contentType: "text/html" }); | |
| const dom = new JSDOM(xml, { contentType: "text/xml" }); |
🤖 Prompt for AI Agents
In `@apps/pipeline/src/tools/generate-book-cli/wrapChaptersWithSections.ts` at
line 20, The JSDOM instance in wrapChaptersWithSections.ts is created with
contentType: "text/html", causing the FB2 XML namespace to be lost; change the
JSDOM constructor call that creates dom (the line with const dom = new
JSDOM(xml, { contentType: ... })) to use contentType: "text/xml" so the original
FB2 namespace and namespaceURIs are preserved and newly created elements via
createElementNS(...) won't get spurious xmlns="" attributes.
| response.characters.push({ | ||
| name: "generic-avatar", | ||
| visualGuide: | ||
| "A mysterious figure shown from behind or in silhouette. No distinct facial features visible. Anonymous, sexless, suitable for representing any unnamed character. Atmospheric lighting with the figure partially obscured by shadow or mist.", | ||
| }); |
There was a problem hiding this comment.
Prevent duplicate generic-avatar entries before push.
An unconditional push can create duplicates and redundant generation/overwrites. Consider guarding with an existence check.
Proposed fix
- response.characters.push({
- name: "generic-avatar",
- visualGuide:
- "A mysterious figure shown from behind or in silhouette. No distinct facial features visible. Anonymous, sexless, suitable for representing any unnamed character. Atmospheric lighting with the figure partially obscured by shadow or mist.",
- });
+ if (!response.characters.some((c) => c.name === "generic-avatar")) {
+ response.characters.push({
+ name: "generic-avatar",
+ visualGuide:
+ "A mysterious figure shown from behind or in silhouette. No distinct facial features visible. Anonymous, sexless, suitable for representing any unnamed character. Atmospheric lighting with the figure partially obscured by shadow or mist.",
+ });
+ }🤖 Prompt for AI Agents
In `@apps/pipeline/src/tools/new-tooling/generate-pictures-for-entities.ts` around
lines 294 - 298, Prevent duplicate "generic-avatar" entries by checking
response.characters for an existing item with name === "generic-avatar" before
calling response.characters.push; locate the push call
(response.characters.push({...}) in generate-pictures-for-entities.ts) and guard
it with a conditional that searches the response.characters array (e.g., using
.some or .find) and only pushes the new object when no matching name exists, so
you avoid redundant entries and duplicate generation/overwrites.
| const paragraphsFromChapter = getParagraphsFromChapter(chapterNum, true, true); | ||
|
|
||
| const paragraphsForPage = paragraphsFromChapter | ||
| .map( | ||
| (paragraph) => | ||
| `<p id="${paragraph.dataIndex}">${paragraph.text.trim().replace(/"/g, "'")}</p>`, | ||
| ) | ||
| .join("\n"); | ||
| const paragraphsForPage = buildParagraphsForSummary(paragraphsFromChapter); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if chunking is handled in getParagraphsFromChapter or elsewhere
echo "=== Checking getParagraphsFromChapter implementation ==="
rg -n "getParagraphsFromChapter" --type=ts apps/pipeline/src/ -B 2 -A 10
echo ""
echo "=== Checking for any chunking utilities in pipeline ==="
rg -n "chunk" --type=ts apps/pipeline/src/ -i
echo ""
echo "=== Checking the specific file being reviewed ==="
cat -n apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts | head -200Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 50387
🏁 Script executed:
#!/bin/bash
# Check what buildParagraphsForSummary does
echo "=== Checking buildParagraphsForSummary implementation ==="
rg -n "buildParagraphsForSummary" --type=ts apps/pipeline/src/ -B 2 -A 15
echo ""
echo "=== Check if there's context limit handling anywhere ==="
rg -n "context.*limit|max.*token|token.*limit" --type=ts apps/pipeline/src/ -iRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 12256
Implement chunking for chapters exceeding the LLM context limit.
This tool processes entire chapters with Gemini and Grok Azure without splitting them into chunks. Per the pipeline guidelines, tools in apps/pipeline/src/tools/**/*.ts should respect LLM context limits by splitting long chapters. Very large chapters may exceed the context window of Grok Azure or Gemini, causing failures or degraded output quality.
Use the existing needsChunking() and chunkParagraphs() utilities from chapterChunker.ts (already imported and used in identifyEntityAndRewriteParagraphs.ts) to split chapters when necessary, following the same pattern as that file.
🤖 Prompt for AI Agents
In
`@apps/pipeline/src/tools/new-tooling/get-chapter-by-chapter-with-paragraphs-json-summary.ts`
around lines 167 - 169, The current flow calls getParagraphsFromChapter(...) and
then buildParagraphsForSummary(...) without splitting large chapters; update
this to detect large inputs using needsChunking(paragraphsFromChapter) and, if
true, call chunkParagraphs(paragraphsFromChapter) and then process each chunk
through buildParagraphsForSummary (or otherwise aggregate per-chunk summaries)
so the downstream LLM calls handle chunked inputs; follow the same pattern used
in identifyEntityAndRewriteParagraphs (use needsChunking and chunkParagraphs
utilities) to locate where to branch and ensure paragraphsForPage is built from
chunked results when required.
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
|
|
||
| if (!trimmed) { | ||
| output.push(line); | ||
| continue; | ||
| } | ||
|
|
||
| if (trimmed.startsWith("<")) { | ||
| output.push(line); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Handle unwrapped lines that begin with inline tags.
Lines starting with <span>/<em> are currently skipped, so paragraphs that begin with inline markup never get rewrapped.
🛠️ Proposed fix
- if (trimmed.startsWith("<")) {
- output.push(line);
- continue;
- }
+ const hasParagraphTag = /<\/?p\b/i.test(trimmed);
+ if (hasParagraphTag) {
+ output.push(line);
+ continue;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const line of lines) { | |
| const trimmed = line.trim(); | |
| if (!trimmed) { | |
| output.push(line); | |
| continue; | |
| } | |
| if (trimmed.startsWith("<")) { | |
| output.push(line); | |
| continue; | |
| } | |
| for (const line of lines) { | |
| const trimmed = line.trim(); | |
| if (!trimmed) { | |
| output.push(line); | |
| continue; | |
| } | |
| const hasParagraphTag = /<\/?p\b/i.test(trimmed); | |
| if (hasParagraphTag) { | |
| output.push(line); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In `@apps/pipeline/src/tools/new-tooling/restore-unwrapped-lines.ts` around lines
68 - 79, The loop in restore-unwrapped-lines.ts currently skips any line where
trimmed.startsWith("<"), which incorrectly skips lines beginning with inline
tags like <span> or <em>; change that check to only skip block-level or special
tags by replacing the trimmed.startsWith("<") test with a more specific
predicate (e.g., a regex or helper like isBlockTag(trimmed)) that matches only
block-level/opening/closing tags and comments (for example:
/^<\/?(?:p|div|h[1-6]|ul|ol|li|pre|code|blockquote|table|thead|tbody|tr|td|th|section|article|header|footer|aside)\b/i
or /^<!--/), leaving lines that start with inline tags to be processed normally
by the remainder of the function (the loop over lines with variables lines,
trimmed, output).
| const tag = html.slice(i, closeIdx + 1); | ||
| const lower = tag.toLowerCase(); | ||
|
|
||
| if (lower.startsWith("<p") && !lower.startsWith("</p")) { | ||
| if (depth >= 1) { | ||
| // Nested <p> -> drop tag, keep content inline | ||
| } else { | ||
| out += tag; | ||
| } | ||
| depth += 1; | ||
| i = closeIdx + 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (lower.startsWith("</p")) { | ||
| if (depth > 1) { | ||
| // Closing nested <p> -> drop tag | ||
| } else { | ||
| out += tag; | ||
| } | ||
| depth = Math.max(0, depth - 1); | ||
| i = closeIdx + 1; |
There was a problem hiding this comment.
Avoid treating <pre>/<picture> as <p> tags.
startsWith("<p") matches any tag beginning with p, which can incorrectly strip non-<p> tags.
🛠️ Proposed fix
- const tag = html.slice(i, closeIdx + 1);
- const lower = tag.toLowerCase();
-
- if (lower.startsWith("<p") && !lower.startsWith("</p")) {
+ const tag = html.slice(i, closeIdx + 1);
+ const isOpenP = /^<p(\s|>)/i.test(tag);
+ const isCloseP = /^<\/p(\s|>)/i.test(tag);
+
+ if (isOpenP) {
if (depth >= 1) {
// Nested <p> -> drop tag, keep content inline
} else {
out += tag;
}
depth += 1;
i = closeIdx + 1;
continue;
}
- if (lower.startsWith("</p")) {
+ if (isCloseP) {
if (depth > 1) {
// Closing nested <p> -> drop tag
} else {
out += tag;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const tag = html.slice(i, closeIdx + 1); | |
| const lower = tag.toLowerCase(); | |
| if (lower.startsWith("<p") && !lower.startsWith("</p")) { | |
| if (depth >= 1) { | |
| // Nested <p> -> drop tag, keep content inline | |
| } else { | |
| out += tag; | |
| } | |
| depth += 1; | |
| i = closeIdx + 1; | |
| continue; | |
| } | |
| if (lower.startsWith("</p")) { | |
| if (depth > 1) { | |
| // Closing nested <p> -> drop tag | |
| } else { | |
| out += tag; | |
| } | |
| depth = Math.max(0, depth - 1); | |
| i = closeIdx + 1; | |
| const tag = html.slice(i, closeIdx + 1); | |
| const isOpenP = /^<p(\s|>)/i.test(tag); | |
| const isCloseP = /^<\/p(\s|>)/i.test(tag); | |
| if (isOpenP) { | |
| if (depth >= 1) { | |
| // Nested <p> -> drop tag, keep content inline | |
| } else { | |
| out += tag; | |
| } | |
| depth += 1; | |
| i = closeIdx + 1; | |
| continue; | |
| } | |
| if (isCloseP) { | |
| if (depth > 1) { | |
| // Closing nested <p> -> drop tag | |
| } else { | |
| out += tag; | |
| } | |
| depth = Math.max(0, depth - 1); | |
| i = closeIdx + 1; |
🤖 Prompt for AI Agents
In `@apps/pipeline/src/tools/new-tooling/sanitize-nested-paragraphs.ts` around
lines 20 - 41, The checks using lower.startsWith("<p") and
lower.startsWith("</p") are too broad and match tags like <pre> or <picture>;
update the conditional logic around the tag/lower variables in
sanitize-nested-paragraphs.ts so the opening check only matches an actual <p>
tag (e.g. require "<p" followed immediately by whitespace or '>' using a regex
like /^<p(?=\s|>)/i) and the closing check only matches a real </p> (e.g.
/^<\/p(?=\s|>)/i); apply these stricter matches where tag/lower are used to
decide nesting (both the opening-branch that currently uses
lower.startsWith("<p") and the closing-branch that uses lower.startsWith("</p"))
so <pre>, <picture>, etc. are not treated as paragraph tags.
| // Handle unknown characters (not in Convex) | ||
| if (!characterData) { | ||
| console.warn(`[populateInlineAvatarShell] ${characterSlug}: no characterData provided`); | ||
| return false; | ||
| console.log("generic", genericCharacter); | ||
| // Try generic avatar first, then SVG fallback | ||
| const genericAvatarUrl = genericCharacter?.media?.avatarUrl; | ||
| console.log(`[populateInlineAvatarShell] Unknown character "${characterSlug}":`, { | ||
| hasGenericCharacter: !!genericCharacter, | ||
| genericCharacterSlug: genericCharacter?.slug, | ||
| genericAvatarUrl, | ||
| genericMedia: genericCharacter?.media, | ||
| }); | ||
| const fallbackSrc = genericAvatarUrl ?? generateFallbackAvatarUrl(characterSlug); | ||
|
|
||
| // Generate display name from slug: "other-board-members" -> "Other Board Members" | ||
| const displayName = characterSlug.replace(/-/g, " ").replace(/\b\w/g, (c) => c.toUpperCase()); | ||
|
|
||
| shell.title = displayName; | ||
|
|
||
| const placeholderImg = document.createElement("img"); | ||
| placeholderImg.src = normalizeSrcForInlineAvatar(fallbackSrc); | ||
| placeholderImg.classList.add( | ||
| "absolute", | ||
| "top-0", | ||
| "left-0", | ||
| "w-full", | ||
| "h-full", | ||
| "object-cover", | ||
| "rounded-full", | ||
| ); | ||
| placeholderImg.alt = displayName; | ||
| shell.appendChild(placeholderImg); | ||
| return true; |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
Lines 331 and 334-339 contain verbose debug logging that should be removed or converted to conditional debug-level logging. These logs expose implementation details and will clutter the browser console in production.
🧹 Proposed fix to remove debug logs
// Handle unknown characters (not in Convex)
if (!characterData) {
- console.log("generic", genericCharacter);
// Try generic avatar first, then SVG fallback
const genericAvatarUrl = genericCharacter?.media?.avatarUrl;
- console.log(`[populateInlineAvatarShell] Unknown character "${characterSlug}":`, {
- hasGenericCharacter: !!genericCharacter,
- genericCharacterSlug: genericCharacter?.slug,
- genericAvatarUrl,
- genericMedia: genericCharacter?.media,
- });
const fallbackSrc = genericAvatarUrl ?? generateFallbackAvatarUrl(characterSlug);
// Generate display name from slug: "other-board-members" -> "Other Board Members"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle unknown characters (not in Convex) | |
| if (!characterData) { | |
| console.warn(`[populateInlineAvatarShell] ${characterSlug}: no characterData provided`); | |
| return false; | |
| console.log("generic", genericCharacter); | |
| // Try generic avatar first, then SVG fallback | |
| const genericAvatarUrl = genericCharacter?.media?.avatarUrl; | |
| console.log(`[populateInlineAvatarShell] Unknown character "${characterSlug}":`, { | |
| hasGenericCharacter: !!genericCharacter, | |
| genericCharacterSlug: genericCharacter?.slug, | |
| genericAvatarUrl, | |
| genericMedia: genericCharacter?.media, | |
| }); | |
| const fallbackSrc = genericAvatarUrl ?? generateFallbackAvatarUrl(characterSlug); | |
| // Generate display name from slug: "other-board-members" -> "Other Board Members" | |
| const displayName = characterSlug.replace(/-/g, " ").replace(/\b\w/g, (c) => c.toUpperCase()); | |
| shell.title = displayName; | |
| const placeholderImg = document.createElement("img"); | |
| placeholderImg.src = normalizeSrcForInlineAvatar(fallbackSrc); | |
| placeholderImg.classList.add( | |
| "absolute", | |
| "top-0", | |
| "left-0", | |
| "w-full", | |
| "h-full", | |
| "object-cover", | |
| "rounded-full", | |
| ); | |
| placeholderImg.alt = displayName; | |
| shell.appendChild(placeholderImg); | |
| return true; | |
| // Handle unknown characters (not in Convex) | |
| if (!characterData) { | |
| // Try generic avatar first, then SVG fallback | |
| const genericAvatarUrl = genericCharacter?.media?.avatarUrl; | |
| const fallbackSrc = genericAvatarUrl ?? generateFallbackAvatarUrl(characterSlug); | |
| // Generate display name from slug: "other-board-members" -> "Other Board Members" | |
| const displayName = characterSlug.replace(/-/g, " ").replace(/\b\w/g, (c) => c.toUpperCase()); | |
| shell.title = displayName; | |
| const placeholderImg = document.createElement("img"); | |
| placeholderImg.src = normalizeSrcForInlineAvatar(fallbackSrc); | |
| placeholderImg.classList.add( | |
| "absolute", | |
| "top-0", | |
| "left-0", | |
| "w-full", | |
| "h-full", | |
| "object-cover", | |
| "rounded-full", | |
| ); | |
| placeholderImg.alt = displayName; | |
| shell.appendChild(placeholderImg); | |
| return true; |
🤖 Prompt for AI Agents
In `@apps/player/src/ui/activateMediaInRange.ts` around lines 329 - 360, Remove
the two debug console.log calls inside the unknown-character branch of
populateInlineAvatarShell (the lines logging "generic" and the
`[populateInlineAvatarShell] Unknown character` object); either delete them or
replace them with a conditional/debug-level logger if an application debug
logger exists, ensuring no verbose console output remains for
genericCharacter/genericAvatarUrl/genericMedia in production while leaving the
rest of the fallback avatar construction (generateFallbackAvatarUrl,
normalizeSrcForInlineAvatar, shell.title assignment, and placeholderImg
creation) unchanged.
| }; | ||
|
|
||
| export function hydrateInlineAvatarsInSection(section: HTMLElement): void { | ||
| console.log("getCharactersData", getCharactersData()); |
There was a problem hiding this comment.
Remove debug console.log statement.
This debug log at line 715 should be removed before merging.
🧹 Proposed fix
export function hydrateInlineAvatarsInSection(section: HTMLElement): void {
- console.log("getCharactersData", getCharactersData());
const charactersBySlug = new Map(getCharactersData().map((c) => [c.slug, c]));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("getCharactersData", getCharactersData()); | |
| export function hydrateInlineAvatarsInSection(section: HTMLElement): void { | |
| const charactersBySlug = new Map(getCharactersData().map((c) => [c.slug, c])); |
🤖 Prompt for AI Agents
In `@apps/player/src/ui/activateMediaInRange.ts` at line 715, Remove the stray
debug console.log in activateMediaInRange.ts that prints getCharactersData();
locate the console.log("getCharactersData", getCharactersData()) call (inside
the activateMediaInRange flow) and delete it or replace it with the appropriate
production logger if needed (e.g., use existing processLogger or debug util) so
no console.debug remains in the merged code.
| const parser = new DOMParser(); | ||
| const doc = parser.parseFromString(htmlResult.content, "text/xml"); | ||
| const doc = parser.parseFromString(htmlResult.content, "text/html"); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git ls-files convex/paragraphEditor.tsRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 145
🏁 Script executed:
head -50 convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1489
🏁 Script executed:
sed -n '240,250p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 512
🏁 Script executed:
sed -n '1,30p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 910
🏁 Script executed:
rg "DOMParser|XMLSerializer|@xmldom" convex/paragraphEditor.ts -A 2 -B 2Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1435
🏁 Script executed:
sed -n '240,260p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 834
🏁 Script executed:
sed -n '340,360p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 902
🏁 Script executed:
sed -n '450,470p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1012
🏁 Script executed:
sed -n '545,560p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 722
🌐 Web query:
@xmldom/xmldom parseFromString text/html behavior document wrappers
💡 Result:
In @xmldom/xmldom, new DOMParser().parseFromString(source, mimeType) behaves like the web DOMParser in the sense that the mimeType is what decides “HTML vs XML” parsing (not the namespace), and newer releases tightened this behavior significantly. (dev.to)
What text/html does (and why you see “wrappers”)
- When you pass
mimeType = "text/html", the parser uses HTML parsing rules and returns a Document representing an HTML document (so it has the usual HTML-document shape, including access likedoc.body). (developer.mozilla.org) - Because it’s an HTML document, if your input is just a fragment like
<div>Hi</div>, it will effectively live under the document’s HTML structure (the typical “wrappers” people notice are the document’shtml/head/bodystructure, and your nodes end up in the body). This is consistent with the platform API expectation that HTML parsing yields an HTMLDocumentyou can query via.body. (developer.mozilla.org)
xmldom-specific notes that commonly trip people up
- In
@xmldom/xmldomv0.9.0+,mimeTypeis mandatory and invalid/unsupported values are rejected (TypeError). (dev.to) - The project explicitly says it does not aim to fully support the HTML spec, only “some capability” when parsing as HTML. If you need highly browser-faithful HTML parsing, you may want a dedicated HTML parser/DOM (e.g., jsdom/parse5/cheerio). (github.com)
How to deal with the wrappers
If your goal is “parse HTML fragment but don’t keep <html><head><body>… in the output”, the usual approach is:
- Parse with
text/html - Work from
doc.body(or its childNodes) - Serialize just the body’s contents / children, not the whole document
This matches the standard usage pattern shown for text/html parsing (doc.body.firstChild…). (developer.mozilla.org)
Citations:
- 1: https://dev.to/karfau/release-090-of-xmldomxmldom-4106?utm_source=openai
- 2: https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString?utm_source=openai
- 3: https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString?utm_source=openai
- 4: https://dev.to/karfau/release-090-of-xmldomxmldom-4106?utm_source=openai
- 5: https://github.com/xmldom/xmldom?utm_source=openai
- 6: https://developer.mozilla.org/en-US/docs/Web/API/DOMParser/parseFromString?utm_source=openai
🌐 Web query:
@xmldom/xmldom text/html vs xml serialization wrapper elements
💡 Result:
With @xmldom/xmldom, the wrapper elements you’re seeing are usually a consequence of parsing as HTML (text/html) and then serializing the whole Document.
Why text/html introduces “wrapper” elements
When you call DOMParser.parseFromString(source, "text/html"), the input is parsed using HTML document rules, which produce a full HTML document tree (i.e., an html root with head and body in the document structure, even if they weren’t present in the string). MDN explicitly distinguishes text/html (HTML parsing) from the XML MIME types (XML parsing). [1]
So if you then do:
new XMLSerializer().serializeToString(doc)you’re serializing the entire HTML document tree, so you’ll naturally get something like:
<html><head>…</head><body>…</body></html>Why XML MIME types don’t add wrappers
If you instead parse as XML (text/xml, application/xml, application/xhtml+xml, image/svg+xml), it’s parsed as an XML document, and the root element is whatever your XML actually contains—no implicit html/head/body document scaffolding is added. [1]
Serialization differences (HTML vs XML)
The DOM Parsing/Serialization spec notes there are HTML vs XML serialization differences, especially around namespaces and empty-element syntax, even though the XML serialization is designed to be HTML-parser-compatible. [2] (So switching MIME type can change more than just wrappers.)
How to avoid wrapper elements
- If your input is really XML (or XHTML-as-XML): parse with an XML MIME type:
const doc = new DOMParser().parseFromString(src, "application/xml"); // or text/xml[1]
- If your input is HTML but you only want the “inner content”: serialize a subtree, not the whole
Document:
const doc = new DOMParser().parseFromString(src, "text/html");
const out = new XMLSerializer().serializeToString(doc.body); // or doc.documentElement, or a specific nodeserializeToString() serializes whatever node you pass as the root. [3]
Sources: MDN DOMParser.parseFromString() (HTML vs XML MIME types) [1], W3C DOM Parsing & Serialization spec (HTML vs XML serialization differences) [2], MDN XMLSerializer.serializeToString() [3].
🏁 Script executed:
find . -name "AGENTS.md" -o -name "*convex*" -type f -name "*.md" | head -20Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 277
🏁 Script executed:
rg "htmlResult|chapter.*content|Chapter" convex/paragraphEditor.ts -B 2 -A 2 | head -60Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1696
🏁 Script executed:
find convex -name "*.ts" -type f | xargs rg "schema.*Chapter" -lRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 59
🏁 Script executed:
rg "Chapter.*html|content.*html" convex/*.ts -B 3 -A 3 | head -80Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 3890
🏁 Script executed:
sed -n '1,100p' convex/chapterCompiler.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 3873
🏁 Script executed:
rg "htmlContent" convex/chapterCompiler.ts -B 5 -A 5 | head -100Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 838
🏁 Script executed:
rg "function.*Compiler|export" convex/chapterCompiler.ts | head -20Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 127
🏁 Script executed:
cat convex/AGENTS.mdRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1261
🏁 Script executed:
rg "section|paragraph|root|wrapper" convex/AGENTS.mdRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 216
🏁 Script executed:
find docs -name "*.md" -type f | xargs rg "chapter.*html|html.*chapter" -lRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 95
🏁 Script executed:
cat docs/conventions/convex-patterns.md | head -150Repository: TheBrainFamily/BookgeniusPlayer
Length of output: 1128
🏁 Script executed:
sed -n '260,280p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 989
🏁 Script executed:
sed -n '280,310p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 1109
🏁 Script executed:
sed -n '243,265p' convex/paragraphEditor.ts | cat -nRepository: TheBrainFamily/BookgeniusPlayer
Length of output: 894
Serializing the full document after text/html parsing persists unwanted <html><head><body> wrappers into stored chapter HTML.
parseFromString(…, "text/html") creates a complete HTML document structure. Calling serializeToString(doc) on the full document includes these wrappers, corrupting the stored HTML shape across all four handlers (lines 243–245, 347–349, 453–455, 549–551).
Extract and serialize body content only:
🔧 Suggested helper for body-only serialization
+function serializeHtmlDocument(doc: XmlDocument, serializer: XMLSerializer): string {
+ const body = doc.getElementsByTagName("body")[0];
+ if (!body) return serializer.serializeToString(doc);
+ return Array.from(body.childNodes)
+ .map((node) => serializer.serializeToString(node))
+ .join("");
+}
const serializer = new XMLSerializer();
- const modifiedHtml = serializer.serializeToString(doc);
+ const modifiedHtml = serializeHtmlDocument(doc, serializer);🤖 Prompt for AI Agents
In `@convex/paragraphEditor.ts` around lines 243 - 245, The current flow parses
htmlResult.content with new DOMParser().parseFromString(..., "text/html") and
then serializes the entire document, which preserves unwanted <html><head><body>
wrappers; update each place where parser and doc are used (where you call
parseFromString on htmlResult.content) to extract and serialize only the
document body—e.g., obtain doc.body and produce the stored HTML from doc.body
(via innerHTML or an XMLSerializer on doc.body) instead of serializing the whole
document—apply this change to all four handlers that use parser/doc to ensure
only body content is persisted.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes