Conversation
handle inline icons with a markdown plugin
📝 WalkthroughWalkthroughThis PR refactors the icon rendering pipeline from Handlebars helpers to a Markdown remark plugin, updates Wellington's aura text formatting across multiple playbook versions and languages, introduces template resolution for character text fields, refactors CSS typography rules for multilingual card display, and switches Vite configuration from Cloudflare to basic SSL plugin. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/models/gbdbTypes.ts (1)
283-293:⚠️ Potential issue | 🟠 MajorFix parameter assignment when trait lookups return null entries.
The
paramsarray maintains original indices, butctsis a filtered array with removed nulls. At line 283,params[index]uses the filteredctsindex instead of the original position, causing parameters to be assigned to the wrong traits when any trait lookup fails.When a trait is missing (e.g.,
characterTraits[1]is null), the filteredctsarray shrinks, butparamsstays the same length. Subsequent parameters then reference wrong indices—for instance,cts[1]would getparams[1]instead ofparams[2].Store the original indices when filtering, or sync the parameter array with the filtered traits:
const cts = characterTraits.map((ct, i) => ct !== null ? { ct, index: i } : null).filter(x => x !== null); const result = await Promise.all(cts.map(async ({ ct, index }) => { const trait = Object.assign(ct.toMutableJSON(), { parameter: params[index], }); // ... }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/gbdbTypes.ts` around lines 283 - 293, The params array is being indexed using the filtered cts index, causing mismatched parameters when characterTraits contains nulls; modify the filtering of characterTraits (cts) to retain original indices (e.g., map to {ct, index} and filter non-null) and then in the Promise.all map use that original index to pull the correct parameter into the trait (set trait.parameter = params[index]); update places referencing cts.map(...) and the trait construction in this block (and still call resolveText(trait.text, db) and throw PartialError as before).
🧹 Nitpick comments (8)
src/pages/App.tsx (1)
85-93: Consider removing stale commented-out gradient alternatives.The background styling changes look fine. However, there are multiple commented-out gradient options (lines 87-92) accumulating as dead code. If these are no longer candidates for the design, removing them would improve readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/App.tsx` around lines 85 - 93, Remove the stale commented-out gradient alternatives in the style block that sets backgroundImage in App.tsx (the commented lines above the "radial-gradient..." value) to clean up dead code; leave only the active backgroundColor and the chosen radial-gradient value in the style object (ensure punctuation/commas around backgroundImage remain valid after deletion).vite.config.ts (2)
14-16: Delete the commentedesbuildblock for a single source of truth.Line 14–16 is dead commented config. Removing it will make
optimizeDeps.esbuildOptionsthe only active target config path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 14 - 16, Remove the dead commented esbuild block (the commented lines containing "esbuild: { target: 'es2022' }") so the project uses a single source of truth for build target configuration; keep only the active optimizeDeps.esbuildOptions configuration and delete the commented block referencing esbuild to avoid confusion.
6-7: Remove commented-out plugin lines to avoid config drift.At Line 6 and Line 23, the old path is retained as comments. Prefer deleting inactive config instead of comment-toggling; it keeps the active plugin chain unambiguous.
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, Remove the commented-out plugin import and any other commented plugin lines (e.g., the "// import { cloudflare } from \"@cloudflare/vite-plugin\";" and the commented lines around the plugin chain) so the active plugin list only contains real imports like basicSsl; locate these comments near the top where the import is present and again in the plugin array/chain (references: the commented cloudflare import and the plugin list that includes basicSsl) and delete them to avoid config drift and keep the config unambiguous.package.json (1)
64-64: Remove the stale@cloudflare/vite-plugindependency.The Cloudflare plugin has been fully disabled in
vite.config.ts(both import and usage are commented out), replaced bybasicSsl(). The dependency remains indevDependenciesbut is no longer used. Removing it reduces maintenance burden and audit surface.Proposed cleanup
"devDependencies": { - "@cloudflare/vite-plugin": "^1.29.0", "@eslint/js": "^10.0.1", "@types/color": "^4.2.0", "@types/file-saver": "^2.0.7", "@types/react": "^19.2.14", "@types/react-detect-offline": "^2.4.6", "@types/react-dom": "^19.2.2", "@vitejs/plugin-basic-ssl": "^2.3.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 64, Remove the unused devDependency "@cloudflare/vite-plugin" from package.json's devDependencies (it is no longer imported or used in vite.config.ts and was replaced by basicSsl()); after removing the entry, run the project's package manager to update lockfile (npm install / yarn install / pnpm install) so the dependency is removed from node_modules and locks.scripts/check.ts (1)
90-101: Consider adding a brief JSDoc comment explaining the equivalence semantics.The function logic is correct for comparing names that may differ only in bracket-suffixed content (e.g.,
"Master Chef [Aura 4"]vs"Master Chef [Aura 4\"]"). However, a short comment would help future maintainers understand what "equivalent" means in this context.Note: Empty prefixes match (e.g.,
"[foo]"≡"[bar]"), and whitespace differences in the prefix (e.g.,"foo [x]"vs"foo[x]") will cause a mismatch. Verify this matches intent.📝 Optional: Add JSDoc for clarity
+/** + * Treats two strings as equivalent if they share the same prefix + * before the first `[` character, allowing bracket-suffixed annotations + * (e.g., aura text) to differ between base and translated strings. + */ function isEquivalentName(base: string, trans: string): boolean {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check.ts` around lines 90 - 101, Add a short JSDoc above the isEquivalentName function that explains the equivalence semantics: treat two names as equivalent if they are strictly equal or if both have a "[" and their prefixes (substring before the first "[") are equal, noting that empty prefixes match and that whitespace differences in the prefix will cause a mismatch; place the comment immediately above the isEquivalentName declaration to help future maintainers understand the bracket-suffix rule.src/components/Gameplan.tsx (1)
129-136: Conditionally render the detail block whengameplan.detailis present.While
CardTextgracefully handles undefined children by returning null, the empty<div className='detail'>is still rendered to the DOM. Wrapping the entire block with a conditional prevents unnecessary DOM nodes when there's no detail content.♻️ Suggested improvement
- <div - className='detail' - id={gameplan.title.replace(/[^a-zA-Z0-9]+/g, '')} - > - <CardText> - {gameplan.detail ? `(_${gameplan.detail}_)` : undefined} - </CardText> - </div> + {gameplan.detail && ( + <div + className='detail' + id={gameplan.title.replace(/[^a-zA-Z0-9]+/g, '')} + > + <CardText> + {`(_${gameplan.detail}_)`} + </CardText> + </div> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Gameplan.tsx` around lines 129 - 136, The div.detail (with id generated from gameplan.title) is rendered even when gameplan.detail is falsy; change the JSX so the entire block containing the div, id generation, and CardText is only rendered when gameplan.detail is truthy (e.g., wrap the div and its CardText in a conditional using gameplan.detail && ...), keeping the id generation logic (gameplan.title.replace(...)) and the CardText usage intact.src/utils/handlebars.ts (2)
61-65: Consider adding type safety for thedbparameter.The
dbparameter is typed asunknown, but it's passed directly to the template context where the helpers expect it to be aGBDatabase. While this works at runtime, it reduces type safety at compile time.💡 Suggested improvement
-export async function resolveText(text: string | undefined, db: unknown, depth: number = 1): Promise<string> { +export async function resolveText(text: string | undefined, db: GBDatabase, depth: number = 1): Promise<string> { if (!text) return ""; const template = hb.compile(text); return await template({ db, depth }); }🤖 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, The resolveText function currently types the db parameter as unknown; change its signature to accept the concrete GBDatabase type (or a generic constrained to GBDatabase) so the template context has correct typings (update the resolveText declaration and any callers to pass GBDatabase), and add/import the GBDatabase type where needed so helpers that read properties from db get compile-time safety while keeping the same runtime behavior in the function.
12-32: Qualifier extraction logic may be incorrect.The check
rest.length > 1to extract the qualifier assumes that if there's more than one item inrest, the first is the qualifier. However, in Handlebars, the options hash object is always the last argument. Withrest.length > 1, you're checking if there are additional positional arguments beyond just the options object.If the template is
{{trait "Name" "qualifier"}}, thenrestwould be["qualifier", optionsObject], makingrest.length === 2, sorest[0]would correctly be"qualifier". This appears correct.However, if no qualifier is provided (
{{trait "Name"}}),restwould be[optionsObject]withrest.length === 1, so the condition correctly returnsundefined.The logic is correct, but consider adding a comment to clarify the argument structure for maintainability.
💡 Suggested documentation
hb.registerHelper("trait", async function (this: HelperContext | undefined, name: string, ...rest: unknown[]) { const db = this?.db; if (!db) { console.error(`Database not provided for trait lookup: ${name}`); return ""; } const trait = await db.character_traits.findOne(name).exec().then(doc => doc?.toJSON()); if (!trait) { console.error(`can not find trait ${name}`); return ""; } + // rest contains [qualifier?, optionsObject] - qualifier is present if rest.length > 1 const qualifier = (rest.length > 1) ? rest[0] as string : undefined;Also applies to: 34-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/handlebars.ts` around lines 12 - 32, The qualifier extraction is fragile/unclear; update the trait helper (hb.registerHelper("trait")) to explicitly handle Handlebars' arguments by checking that the last item in rest is the options hash and, if any positional args remain, use rest[0] as the qualifier, and add a short comment explaining that rest contains [positionalArgs..., optionsObject] so future readers understand why we check/rest[0]; reference the qualifier variable and rest array in your change and ensure behavior for both {{trait "Name"}} and {{trait "Name" "qualifier"}} remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/CardQuirks.css`:
- Around line 174-180: The Stylelint failure is caused by placing a declaration
immediately after a nested rule; in the rules under the selector
.Skatha:lang(es) .character-plays (and the nested .ColdSnap .text and similar
blocks), insert a blank line before each letter-spacing declaration (the lines
setting "letter-spacing: -0.5px" and the other letter-spacing lines referenced
around the .ColdSnap .text blocks) so there is an empty line separating the
nested rule block (e.g., "& ::first-line { ... }") and the subsequent
"letter-spacing" declaration.
---
Outside diff comments:
In `@src/models/gbdbTypes.ts`:
- Around line 283-293: The params array is being indexed using the filtered cts
index, causing mismatched parameters when characterTraits contains nulls; modify
the filtering of characterTraits (cts) to retain original indices (e.g., map to
{ct, index} and filter non-null) and then in the Promise.all map use that
original index to pull the correct parameter into the trait (set trait.parameter
= params[index]); update places referencing cts.map(...) and the trait
construction in this block (and still call resolveText(trait.text, db) and throw
PartialError as before).
---
Nitpick comments:
In `@package.json`:
- Line 64: Remove the unused devDependency "@cloudflare/vite-plugin" from
package.json's devDependencies (it is no longer imported or used in
vite.config.ts and was replaced by basicSsl()); after removing the entry, run
the project's package manager to update lockfile (npm install / yarn install /
pnpm install) so the dependency is removed from node_modules and locks.
In `@scripts/check.ts`:
- Around line 90-101: Add a short JSDoc above the isEquivalentName function that
explains the equivalence semantics: treat two names as equivalent if they are
strictly equal or if both have a "[" and their prefixes (substring before the
first "[") are equal, noting that empty prefixes match and that whitespace
differences in the prefix will cause a mismatch; place the comment immediately
above the isEquivalentName declaration to help future maintainers understand the
bracket-suffix rule.
In `@src/components/Gameplan.tsx`:
- Around line 129-136: The div.detail (with id generated from gameplan.title) is
rendered even when gameplan.detail is falsy; change the JSX so the entire block
containing the div, id generation, and CardText is only rendered when
gameplan.detail is truthy (e.g., wrap the div and its CardText in a conditional
using gameplan.detail && ...), keeping the id generation logic
(gameplan.title.replace(...)) and the CardText usage intact.
In `@src/pages/App.tsx`:
- Around line 85-93: Remove the stale commented-out gradient alternatives in the
style block that sets backgroundImage in App.tsx (the commented lines above the
"radial-gradient..." value) to clean up dead code; leave only the active
backgroundColor and the chosen radial-gradient value in the style object (ensure
punctuation/commas around backgroundImage remain valid after deletion).
In `@src/utils/handlebars.ts`:
- Around line 61-65: The resolveText function currently types the db parameter
as unknown; change its signature to accept the concrete GBDatabase type (or a
generic constrained to GBDatabase) so the template context has correct typings
(update the resolveText declaration and any callers to pass GBDatabase), and
add/import the GBDatabase type where needed so helpers that read properties from
db get compile-time safety while keeping the same runtime behavior in the
function.
- Around line 12-32: The qualifier extraction is fragile/unclear; update the
trait helper (hb.registerHelper("trait")) to explicitly handle Handlebars'
arguments by checking that the last item in rest is the options hash and, if any
positional args remain, use rest[0] as the qualifier, and add a short comment
explaining that rest contains [positionalArgs..., optionsObject] so future
readers understand why we check/rest[0]; reference the qualifier variable and
rest array in your change and ensure behavior for both {{trait "Name"}} and
{{trait "Name" "qualifier"}} remains the same.
In `@vite.config.ts`:
- Around line 14-16: Remove the dead commented esbuild block (the commented
lines containing "esbuild: { target: 'es2022' }") so the project uses a single
source of truth for build target configuration; keep only the active
optimizeDeps.esbuildOptions configuration and delete the commented block
referencing esbuild to avoid confusion.
- Around line 6-7: Remove the commented-out plugin import and any other
commented plugin lines (e.g., the "// import { cloudflare } from
\"@cloudflare/vite-plugin\";" and the commented lines around the plugin chain)
so the active plugin list only contains real imports like basicSsl; locate these
comments near the top where the import is present and again in the plugin
array/chain (references: the commented cloudflare import and the plugin list
that includes basicSsl) and delete them to avoid config drift and keep the
config unambiguous.
🪄 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: 22d8c12b-fa54-46f4-ae5b-9371394f19ad
⛔ 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
| .Skatha:lang(es) .character-plays { | ||
| & .text { | ||
| font-size: calc(6.6pt * 200/96); | ||
| } | ||
| & .ColdSnap .text { | ||
| & ::first-line { letter-spacing: normal; } | ||
| letter-spacing: -0.5px; |
There was a problem hiding this comment.
Stylelint will keep failing on these nested blocks.
Line 180, Line 203, and Line 227 place a declaration immediately after a nested rule, which trips declaration-empty-line-before. Add a blank line before each letter-spacing declaration.
🧹 Minimal lint 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-203, 225-227
🧰 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 174 - 180, The Stylelint failure
is caused by placing a declaration immediately after a nested rule; in the rules
under the selector .Skatha:lang(es) .character-plays (and the nested .ColdSnap
.text and similar blocks), insert a blank line before each letter-spacing
declaration (the lines setting "letter-spacing: -0.5px" and the other
letter-spacing lines referenced around the .ColdSnap .text blocks) so there is
an empty line separating the nested rule block (e.g., "& ::first-line { ... }")
and the subsequent "letter-spacing" declaration.
2 participants
Summary by CodeRabbit
Bug Fixes
Style
Chores