Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
handle inline icons with a markdown plugin
cleech
force-pushed
the
translation-fixes
branch
from
e1a7e22 to
318d5c7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/pages/App.tsx (1)
60-61: Consider removing long commented-out style variants.Keeping deprecated color options and the disabled
&::beforeblock inline adds noise and makes future theme edits harder to scan.♻️ Optional cleanup
primary: { main: "#4e91ba", - // main: "#ba9d4e", - // main: "#5578a6", }, @@ body: { - // need this hackery to allow nested backdrop-filters ... - // '&::before': { - // content: "''", - // position: "absolute", - // width: "100%", - // height: "100%", - // backdropFilter: "blur(20px)", - // zIndex: -99, - // }, // overflow: "hidden",Also applies to: 76-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/App.tsx` around lines 60 - 61, Remove the long commented-out style variants to reduce visual noise in App.tsx: delete the commented color lines like the alternate "main" values (e.g., // main: "#ba9d4e", // main: "#5578a6") and the disabled &::before block, or if you want to preserve them, move them into a single clearly named fallback/legacy-theme constant (e.g., LEGACY_THEME_VARIANTS) or a short note in a changelog instead of inline comments; update any references in the theme-building code (the theme/variant creation logic in App.tsx) to use the live values only.vite.config.ts (1)
6-7: Consider removing commented-out code or adding clarifying comments.Leaving the Cloudflare plugin import and invocation commented out rather than removed suggests this may be for easy toggling between configurations. If intentional, consider adding a brief comment explaining when to use each plugin (e.g.,
// Use cloudflare() for production preview, basicSsl() for local dev).Also verify that commenting out
cloudflare()doesn't affect production deployment, sincewrangler deployis still inpackage.jsonscripts and@cloudflare/vite-pluginremains in devDependencies.Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 6 - 7, The commented-out import and usage of cloudflare (the symbol "cloudflare") next to the active import of basicSsl (the symbol "basicSsl") should either be removed or annotated: either delete the commented import/invocation to avoid stale code, or add a short clarifying comment above them (e.g., when to use cloudflare() vs basicSsl() for prod preview vs local dev) so future readers know it's intentional; also scan the "wrangler deploy" usage in package.json and devDependencies to ensure keeping the commented cloudflare() won't cause confusion for production deployment and update package.json or comments accordingly.src/utils/handlebars.ts (2)
34-54: Consider extracting shared logic betweentraitandplayhelpers.Both helpers have nearly identical structure: db validation, document lookup, qualifier extraction, depth-based resolution, and SafeString formatting. A shared utility function could reduce duplication.
♻️ Example extraction
async function lookupAndFormat( context: HelperContext | undefined, collection: 'character_traits' | 'character_plays', name: string, rest: unknown[] ): Promise<string | Handlebars.SafeString> { const db = context?.db; if (!db) { console.error(`Database not provided for ${collection} lookup: ${name}`); return ""; } const doc = await db[collection].findOne(name).exec(); const item = doc?.toJSON(); if (!item) { console.error(`can not find ${collection.replace('character_', '')} ${name}`); return ""; } const qualifier = (rest.length > 1) ? rest[0] as string : undefined; const depth = context?.depth || 1; const text = depth === 1 ? await resolveText(item.text, db, 2) : item.text; return new hb.SafeString(`(_${item.name}${qualifier ? ` [${qualifier}]` : ''}: ${text}_)`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/handlebars.ts` around lines 34 - 54, Extract the duplicated logic in the "play" and "trait" helpers into a shared async utility (e.g., lookupAndFormat) that accepts (context: HelperContext | undefined, collectionName: 'character_traits' | 'character_plays', name: string, rest: unknown[]) and performs db presence check (this?.db), document lookup via db[collectionName].findOne(name).exec(), doc->toJSON(), qualifier extraction ((rest.length > 1) ? rest[0] as string : undefined), depth resolution (context?.depth || 1) and conditional text resolution via resolveText when depth === 1; have the helper functions (play and trait) call this utility and return the resulting Handlebars.SafeString so you can remove the duplicated blocks that reference db.character_plays, play.text, play.name, hb.SafeString and similar code in the trait helper.
18-18: Simplify by removing redundant.then()afterawait.Mixing
awaitand.then()is unnecessarily verbose here.♻️ Suggested simplification
- const trait = await db.character_traits.findOne(name).exec().then(doc => doc?.toJSON()); + const doc = await db.character_traits.findOne(name).exec(); + const trait = doc?.toJSON();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/handlebars.ts` at line 18, The line that assigns trait mixes await with .then unnecessarily; change the expression so you await the promise returned by db.character_traits.findOne(name).exec() and then call .toJSON() on the result (e.g. const doc = await db.character_traits.findOne(name).exec(); const trait = doc?.toJSON();), updating the variable located where trait is assigned and keeping references to db.character_traits.findOne, exec, and toJSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@public/data/GB-Playbook-4-8.es.json`:
- Line 12959: Update the Spanish text entry in GB-Playbook-4-8.es.json that
currently reads "terminen su activación" to use the plural form "terminen sus
activaciones" for consistency with adjacent entries; locate the JSON object
whose "text" value contains "Coloca un AOE duradero dentro del alcance. Los
modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que
entren o terminen su activación dentro de este AOE duradero reciben la condición
de Burning." and replace that substring accordingly, preserving the rest of the
sentence and punctuation.
In `@src/components/CardQuirks.css`:
- Around line 178-181: The CSS rule blocks for the selector ".ColdSnap .text"
(and the similar blocks at the other occurrences) are triggering stylelint's
declaration-empty-line-before; fix by inserting a blank line before the
declaration lines inside those blocks so each property (e.g., "letter-spacing:
normal;" for "::first-line" and "letter-spacing: -0.5px;") is preceded by an
empty line, ensuring consistent formatting and satisfying the linter (adjust the
same change for the occurrences around the other selectors referenced).
In `@src/components/Gameplan.tsx`:
- Around line 125-135: The CardText markdown pipeline currently uses rehypeRaw
without sanitization; update src/components/CardUtils.tsx by importing
rehypeSanitize from 'rehype-sanitize' and adding it to the rehypePlugins array
used by the CardText renderer (alongside rehypeRaw) so raw HTML in gameplan.text
and gameplan.detail is sanitized before rendering, ensuring the CardText
component's pipeline (rehypePlugins, rehypeRaw) includes rehypeSanitize.
In `@src/utils/handlebars.ts`:
- Around line 61-65: Update the resolveText function signature to use the
concrete GBDatabase type instead of unknown: change the parameter declaration in
resolveText(db: unknown) to db: GBDatabase to match the existing HelperContext
and helper usage, and ensure the call sites provide a GBDatabase instance; the
body can remain the same (using hb.compile and template({ db, depth })), and
consider adding a note/comment to cache compiled templates if repeated
compilation becomes a performance concern.
---
Nitpick comments:
In `@src/pages/App.tsx`:
- Around line 60-61: Remove the long commented-out style variants to reduce
visual noise in App.tsx: delete the commented color lines like the alternate
"main" values (e.g., // main: "#ba9d4e", // main: "#5578a6") and the disabled
&::before block, or if you want to preserve them, move them into a single
clearly named fallback/legacy-theme constant (e.g., LEGACY_THEME_VARIANTS) or a
short note in a changelog instead of inline comments; update any references in
the theme-building code (the theme/variant creation logic in App.tsx) to use the
live values only.
In `@src/utils/handlebars.ts`:
- Around line 34-54: Extract the duplicated logic in the "play" and "trait"
helpers into a shared async utility (e.g., lookupAndFormat) that accepts
(context: HelperContext | undefined, collectionName: 'character_traits' |
'character_plays', name: string, rest: unknown[]) and performs db presence check
(this?.db), document lookup via db[collectionName].findOne(name).exec(),
doc->toJSON(), qualifier extraction ((rest.length > 1) ? rest[0] as string :
undefined), depth resolution (context?.depth || 1) and conditional text
resolution via resolveText when depth === 1; have the helper functions (play and
trait) call this utility and return the resulting Handlebars.SafeString so you
can remove the duplicated blocks that reference db.character_plays, play.text,
play.name, hb.SafeString and similar code in the trait helper.
- Line 18: The line that assigns trait mixes await with .then unnecessarily;
change the expression so you await the promise returned by
db.character_traits.findOne(name).exec() and then call .toJSON() on the result
(e.g. const doc = await db.character_traits.findOne(name).exec(); const trait =
doc?.toJSON();), updating the variable located where trait is assigned and
keeping references to db.character_traits.findOne, exec, and toJSON.
In `@vite.config.ts`:
- Around line 6-7: The commented-out import and usage of cloudflare (the symbol
"cloudflare") next to the active import of basicSsl (the symbol "basicSsl")
should either be removed or annotated: either delete the commented
import/invocation to avoid stale code, or add a short clarifying comment above
them (e.g., when to use cloudflare() vs basicSsl() for prod preview vs local
dev) so future readers know it's intentional; also scan the "wrangler deploy"
usage in package.json and devDependencies to ensure keeping the commented
cloudflare() won't cause confusion for production deployment and update
package.json or comments accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfdc3f48-7aac-47fd-807e-32163f469150
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
package.jsonpublic/data/GB-Playbook-4-3.jsonpublic/data/GB-Playbook-4-4.jsonpublic/data/GB-Playbook-4-5.jsonpublic/data/GB-Playbook-4-6.fr.jsonpublic/data/GB-Playbook-4-6.jsonpublic/data/GB-Playbook-4-7.fr.jsonpublic/data/GB-Playbook-4-7.jsonpublic/data/GB-Playbook-4-7.zh.jsonpublic/data/GB-Playbook-4-8.es.jsonpublic/data/GB-Playbook-4-8.fr.jsonpublic/data/GB-Playbook-4-8.jsonpublic/data/GB-Playbook-4-8.zh.jsonpublic/data/manifest.jsonpublic/data/manifest.yamlscripts/check.tssrc/components/CardFront.csssrc/components/CardQuirks.csssrc/components/CardUtils.tsxsrc/components/Gameplan.tsxsrc/models/gbdbTypes.tssrc/pages/App.tsxsrc/pages/GamePlay/components/RosterList.tsxsrc/utils/handlebars.tsvite.config.ts
| { | ||
| "name": "Chilli Cook Out", | ||
| "text": "Coloca un AOE duradero dentro del alcance.\nLos modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que entren o terminen su activación dentro de este AOE duradero reciben la condición de Burning.", | ||
| "text": "Coloca un AOE duradero dentro del alcance. Los modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que entren o terminen su activación dentro de este AOE duradero reciben la condición de Burning.", |
There was a problem hiding this comment.
Use plural form for consistency in activation wording.
Line 12959 uses “terminen su activación”, while adjacent entries consistently use “terminen sus activaciones”. Consider aligning to the plural form for translation consistency.
✏️ Suggested wording tweak
- "text": "Coloca un AOE duradero dentro del alcance. Los modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que entren o terminen su activación dentro de este AOE duradero reciben la condición de Burning.",
+ "text": "Coloca un AOE duradero dentro del alcance. Los modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que entren o terminen sus activaciones dentro de este AOE duradero reciben la condición de Burning.",📝 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.
| "text": "Coloca un AOE duradero dentro del alcance. Los modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que entren o terminen su activación dentro de este AOE duradero reciben la condición de Burning.", | |
| "text": "Coloca un AOE duradero dentro del alcance. Los modelos impactados reciben 4 DAÑO y la condición de Burning. Los modelos que entren o terminen sus activaciones dentro de este AOE duradero reciben la condición de Burning.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/data/GB-Playbook-4-8.es.json` at line 12959, Update the Spanish text
entry in GB-Playbook-4-8.es.json that currently reads "terminen su activación"
to use the plural form "terminen sus activaciones" for consistency with adjacent
entries; locate the JSON object whose "text" value contains "Coloca un AOE
duradero dentro del alcance. Los modelos impactados reciben 4 DAÑO y la
condición de Burning. Los modelos que entren o terminen su activación dentro de
este AOE duradero reciben la condición de Burning." and replace that substring
accordingly, preserving the rest of the sentence and punctuation.
| & .ColdSnap .text { | ||
| & ::first-line { letter-spacing: normal; } | ||
| letter-spacing: -0.5px; | ||
| } |
There was a problem hiding this comment.
Minor: Stylelint expects empty lines before these declarations.
The linter flags declaration-empty-line-before violations. While purely cosmetic, fixing them maintains consistent formatting.
🔧 Proposed fix
& .ColdSnap .text {
& ::first-line { letter-spacing: normal; }
+
letter-spacing: -0.5px;
} & .FlareOut .text {
& ::first-line { letter-spacing: normal; }
+
letter-spacing: -0.7px;
} & .Reconnaissance .text {
& ::first-line { letter-spacing: normal; }
+
letter-spacing: -0.8px;
}Also applies to: 201-204, 225-228
🧰 Tools
🪛 Stylelint (17.5.0)
[error] 180-180: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/CardQuirks.css` around lines 178 - 181, The CSS rule blocks
for the selector ".ColdSnap .text" (and the similar blocks at the other
occurrences) are triggering stylelint's declaration-empty-line-before; fix by
inserting a blank line before the declaration lines inside those blocks so each
property (e.g., "letter-spacing: normal;" for "::first-line" and
"letter-spacing: -0.5px;") is preceded by an empty line, ensuring consistent
formatting and satisfying the linter (adjust the same change for the occurrences
around the other selectors referenced).
| <CardText> | ||
| {gameplan.text} | ||
| </Markdown> | ||
| </CardText> | ||
| </div> | ||
| <div | ||
| className='detail' | ||
| id={gameplan.title.replace(/[^a-zA-Z0-9]+/g, '')} | ||
| > | ||
| <Markdown> | ||
| <CardText> | ||
| {gameplan.detail ? `(_${gameplan.detail}_)` : undefined} | ||
| </Markdown> | ||
| </CardText> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking CardText markdown pipeline configuration..."
fd -i "CardUtils.tsx" src --exec rg -n -C3 "CardText|rehypeRaw|rehype-sanitize|remarkGBIcons|react-markdown|Markdown"
echo
echo "Checking where gameplan/translations content is sourced..."
rg -n -C2 "manifest|translation|translations|gameplan|fetch\\(" src publicRepository: cleech/GBPlaybook
Length of output: 27176
Add rehype-sanitize to the CardText markdown pipeline in src/components/CardUtils.tsx.
The CardText component uses rehypeRaw without rehype-sanitize, which permits raw HTML rendering from gameplan data without sanitization. Since gameplan content (text and detail) is loaded from JSON files and rendered through this pipeline, malicious or compromised data could execute scripts. Import and add rehype-sanitize to the rehypePlugins array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Gameplan.tsx` around lines 125 - 135, The CardText markdown
pipeline currently uses rehypeRaw without sanitization; update
src/components/CardUtils.tsx by importing rehypeSanitize from 'rehype-sanitize'
and adding it to the rehypePlugins array used by the CardText renderer
(alongside rehypeRaw) so raw HTML in gameplan.text and gameplan.detail is
sanitized before rendering, ensuring the CardText component's pipeline
(rehypePlugins, rehypeRaw) includes rehypeSanitize.
| export async function resolveText(text: string | undefined, db: unknown, depth: number = 1): Promise<string> { | ||
| if (!text) return ""; | ||
| const template = hb.compile(text); | ||
| return await template({ db, depth }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there would be circular import issues with GBDatabase type
rg -l "from.*handlebars" --type=ts
rg "import.*resolveText" --type=ts -C2Repository: cleech/GBPlaybook
Length of output: 350
🏁 Script executed:
# Check all imports in handlebars.ts
cat -n src/utils/handlebars.ts | head -20Repository: cleech/GBPlaybook
Length of output: 845
🏁 Script executed:
# Check if GBDatabase is defined in gbdbTypes.ts
rg "export.*interface.*GBDatabase|export.*type.*GBDatabase|export.*class.*GBDatabase" src/models/gbdbTypes.tsRepository: cleech/GBPlaybook
Length of output: 116
🏁 Script executed:
# Check all imports from gbdbTypes in the codebase
rg "import.*GBDatabase" --type=tsRepository: cleech/GBPlaybook
Length of output: 1213
🏁 Script executed:
# Check how db parameter is actually used in the file
rg -A20 "export async function resolveText" src/utils/handlebars.tsRepository: cleech/GBPlaybook
Length of output: 294
Change db: unknown to db: GBDatabase — it's already imported.
The GBDatabase type is already available in this file (line 3). The HelperContext interface within the same file explicitly expects db: GBDatabase (line 8), and the template helpers receive the db parameter through the context object. Using the proper type here removes unnecessary type widening and improves safety.
If repeated template compilation becomes a performance concern, consider caching compiled templates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/handlebars.ts` around lines 61 - 65, Update the resolveText
function signature to use the concrete GBDatabase type instead of unknown:
change the parameter declaration in resolveText(db: unknown) to db: GBDatabase
to match the existing HelperContext and helper usage, and ensure the call sites
provide a GBDatabase instance; the body can remain the same (using hb.compile
and template({ db, depth })), and consider adding a note/comment to cache
compiled templates if repeated compilation becomes a performance concern.
1 participant
Summary by CodeRabbit
Bug Fixes
Style
Chores