Conversation
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
📝 WalkthroughWalkthroughAdds a local Expo app icon badge plugin and a Node.js generator. The plugin creates jobs from configured icon paths, invokes the generator, validates generated PNGs, rewrites icon references, and is covered by enabled and disabled behavior tests. ChangesApp icon badge workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ExpoConfig
participant withAppIconBadge
participant generateAppIconBadges
participant addBadge
ExpoConfig->>withAppIconBadge: provide icon configuration and badge options
withAppIconBadge->>generateAppIconBadges: submit icon jobs and badges
generateAppIconBadges->>addBadge: generate badge image for each job
addBadge-->>generateAppIconBadges: write temporary PNG
generateAppIconBadges-->>withAppIconBadge: provide validated generated outputs
withAppIconBadge-->>ExpoConfig: rewrite icon paths
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (1)
plugins/__tests__/with-app-icon-badge.test.ts (1)
45-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
badgespayload handoff.The test supplies badges but never verifies they reach the generator, so sending an empty or altered badge list would still pass and generate unbadged icons.
Proposed test update
const payload = JSON.parse(generatorArguments[1] ?? '{}') as { + badges: AppIconBadgeConfig['badges']; jobs: { isAdaptiveIcon: boolean; outputPath: string; sourcePath: string }[]; }; + expect(payload.badges).toEqual(options.badges); expect(payload.jobs).toEqual([As per coding guidelines, “Generate tests for all new components, services, and logic.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/__tests__/with-app-icon-badge.test.ts` around lines 45 - 56, Update the withAppIconBadge test around the options, generatorArguments, and parsed payload to assert that the configured badges are included in the generator payload unchanged. Verify the payload’s badges field matches the supplied banner badge so altered or missing badge data fails the test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate-app-icon-badges.js`:
- Line 5: Add jimp as a direct root devDependency for the
generate-app-icon-badges script, keeping its existing direct require in that
script unchanged; do not rely on app-icon-badge’s transitive dependency.
---
Nitpick comments:
In `@plugins/__tests__/with-app-icon-badge.test.ts`:
- Around line 45-56: Update the withAppIconBadge test around the options,
generatorArguments, and parsed payload to assert that the configured badges are
included in the generator payload unchanged. Verify the payload’s badges field
matches the supplied banner badge so altered or missing badge data fails the
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b15aa9c5-a4bc-4fbb-891d-ab9bcf67008a
📒 Files selected for processing (4)
app.config.tsplugins/__tests__/with-app-icon-badge.test.tsplugins/with-app-icon-badge.jsscripts/generate-app-icon-badges.js
| const { dirname } = require('path'); | ||
|
|
||
| const { addBadge } = require('app-icon-badge'); | ||
| const Jimp = require('jimp'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
jq -e '
.dependencies.jimp //
.devDependencies.jimp //
.optionalDependencies.jimp
' package.jsonRepository: Resgrid/IC
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -eu
printf '--- package manager / scripts context ---\n'
jq -r '{type: .type, scripts: .scripts, dependencies, devDependencies, optionalDependencies, packageManager}' package.json 2>/dev/null || true
printf '\n--- jimp references ---\n'
rg -n "require\\(['\"]jimp['\"]\\)|from ['\"]jimp['\"]|jimp" -S --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' .
printf '\n--- lock/package files ---\n'
git ls-files | rg '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb|pnpm-workspace\.yaml)$' || trueRepository: Resgrid/IC
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package manager / scripts context ---'
jq -r '{type: .type, scripts: .scripts, dependencies, devDependencies, optionalDependencies, packageManager}' package.json 2>/dev/null || true
printf '%s\n' ''
printf '%s\n' '--- jimp references ---'
rg -n "require\\(['\"]jimp['\"]\\)|from ['\"]jimp['\"]|jimp" -S --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' .
printf '%s\n' ''
printf '%s\n' '--- lock/package files ---'
git ls-files | rg '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb|pnpm-workspace\.yaml)$' || trueRepository: Resgrid/IC
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import json
from pathlib import Path
pkg = json.loads(Path('package.json').read_text())
name = pkg.get('name')
version = pkg.get('version')
print('package:', name, version)
print('direct_jimp:', pkg.get('dependencies', {}).get('jimp') or pkg.get('devDependencies', {}).get('jimp') or pkg.get('optionalDependencies', {}).get('jimp') or '(none)')
print('direct_app_icon_badge:', pkg.get('dependencies', {}).get('app-icon-badge') or pkg.get('devDependencies', {}).get('app-icon-badge') or '(none)')
lock = Path('yarn.lock').read_text()
for pkg_name in ['"jimp@%', '"app-icon-badge@%']:
print(f'\n--- lock entry: {pkg_name.lstrip(chr(34))} ---')
idx = lock.find(pkg_name)
print(idx)
if idx != -1:
start = lock.rfind('\n', 0, idx)
end = lock.find('\n\n', idx)
print(lock[start:end] if end != -1 else lock[start:start+1200])
PYRepository: Resgrid/IC
Length of output: 287
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scripts/generate-app-icon-badges.js ---'
cat -n scripts/generate-app-icon-badges.js
printf '%s\n' ''
printf '%s\n' '--- app-icon-badge lock entry (loose) ---'
rg -n '^\s*app-icon-badge' yarn.lock -A 8
printf '%s\n' ''
printf '%s\n' '--- jimp references in package files only ---'
python3 - <<'PY'
import json, glob
for path in ['package.json'] + glob.glob('*/*/package.json'):
try:
data=json.load(open(path))
except Exception:
continue
deps={}
for k in ('dependencies','devDependencies','optionalDependencies'):
deps.update(data.get(k,{}))
print(path, {_:deps[_] for _ in deps if _ == 'jimp' or _ == 'app-icon-badge'} or 'no jimp/app-icon-badge')
PYRepository: Resgrid/IC
Length of output: 50366
Declare jimp as a direct script dependency.
scripts/generate-app-icon-badges.js imports jimp directly, but the root package.json only declares app-icon-badge; jimp is only available transitively through that package. Add it under devDependencies since this is a Node script, or change the script to import the image operations from app-icon-badge itself.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/generate-app-icon-badges.js` at line 5, Add jimp as a direct root
devDependency for the generate-app-icon-badges script, keeping its existing
direct require in that script unchanged; do not rely on app-icon-badge’s
transitive dependency.
| execFileSync(process.execPath, [generatorPath, JSON.stringify({ badges: options.badges ?? [], jobs })], { | ||
| cwd: projectRoot, | ||
| stdio: 'inherit', | ||
| }); |
There was a problem hiding this comment.
Uncaught exception in the execFileSync subprocess call propagates raw child-process errors without context when the generator script fails (non-zero exit, missing file, crash). Wrap the call in try/catch, attach context (projectRoot, job count, target paths), and rethrow a mapped application-level Error preserving the original via cause. Also found in scripts/generate-app-icon-badges.js:41.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File plugins/with-app-icon-badge.js:
Line 54 to 57:
Uncaught exception in the `execFileSync` subprocess call propagates raw child-process errors without context when the generator script fails (non-zero exit, missing file, crash). Wrap the call in try/catch, attach context (projectRoot, job count, target paths), and rethrow a mapped application-level Error preserving the original via cause. Also found in scripts/generate-app-icon-badges.js:41.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const { dirname } = require('path'); | ||
|
|
||
| const { addBadge } = require('app-icon-badge'); | ||
| const Jimp = require('jimp'); |
There was a problem hiding this comment.
Missing direct dependency causes require('jimp') at line 5 of scripts/generate-app-icon-badges.js to throw MODULE_NOT_FOUND under pnpm's strict layout (or if app-icon-badge drops jimp), failing the Expo build via execFileSync. Add jimp as an explicit devDependency in package.json, matching the version app-icon-badge resolves.
// package.json (devDependencies):
// "jimp": "<version matching app-icon-badge's resolved jimp>"
const { addBadge } = require('app-icon-badge');
const Jimp = require('jimp');Prompt for LLM
File scripts/generate-app-icon-badges.js:
Line 5:
Missing direct dependency causes `require('jimp')` at line 5 of scripts/generate-app-icon-badges.js to throw MODULE_NOT_FOUND under pnpm's strict layout (or if app-icon-badge drops jimp), failing the Expo build via execFileSync. Add `jimp` as an explicit devDependency in package.json, matching the version app-icon-badge resolves.
Suggested Code:
// package.json (devDependencies):
// "jimp": "<version matching app-icon-badge's resolved jimp>"
const { addBadge } = require('app-icon-badge');
const Jimp = require('jimp');
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| function delay(duration) { | ||
| return new Promise((resolve) => { | ||
| setTimeout(resolve, duration); |
There was a problem hiding this comment.
Uncleared timer in the delay() helper creates a setTimeout with no deterministic cleanup path, keeping the handle alive on teardown or abort. Store the timer id and return a cancelable wrapper exposing clearTimeout, or accept an AbortSignal and clear on abort.
Kody rule violation: Clear timers on teardown/unmount
Prompt for LLM
File scripts/generate-app-icon-badges.js:
Line 13:
Uncleared timer in the `delay()` helper creates a setTimeout with no deterministic cleanup path, keeping the handle alive on teardown or abort. Store the timer id and return a cancelable wrapper exposing clearTimeout, or accept an AbortSignal and clear on abort.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| async function waitForValidPng(imagePath) { | ||
| const startedAt = Date.now(); |
There was a problem hiding this comment.
Wall-clock time from Date.now() is susceptible to system clock adjustments (NTP, manual changes), making elapsed-time calculations unreliable. Use a monotonic clock such as performance.now() or process.hrtime.bigint() to record the start timestamp. Also found in scripts/generate-app-icon-badges.js:20.
Kody rule violation: Avoid `DateTime.Now` for Timing Operations
Prompt for LLM
File scripts/generate-app-icon-badges.js:
Line 18:
Wall-clock time from Date.now() is susceptible to system clock adjustments (NTP, manual changes), making elapsed-time calculations unreliable. Use a monotonic clock such as performance.now() or process.hrtime.bigint() to record the start timestamp. Also found in scripts/generate-app-icon-badges.js:20.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return; | ||
| } | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
Silent exception swallowing in the catch block on line 29 discards every error with only a comment, violating deterministic error handling. Log at debug level with context (e.g., imagePath, err) or narrow the catch to specific expected error codes. Also found in scripts/generate-app-icon-badges.js:56.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File scripts/generate-app-icon-badges.js:
Line 29:
Silent exception swallowing in the catch block on line 29 discards every error with only a comment, violating deterministic error handling. Log at debug level with context (e.g., imagePath, err) or narrow the catch to specific expected error codes. Also found in scripts/generate-app-icon-badges.js:56.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } | ||
|
|
||
| main().catch((error) => { | ||
| console.error('Failed to generate app icon badges:', error); |
There was a problem hiding this comment.
Unstructured error logging via console.error with a plain message string omits the operation name and relevant identifiers required for searchability and correlation. Emit a structured record including op, err.message, and stack, or use a structured logger with fields.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File scripts/generate-app-icon-badges.js:
Line 74:
Unstructured error logging via `console.error` with a plain message string omits the operation name and relevant identifiers required for searchability and correlation. Emit a structured record including op, err.message, and stack, or use a structured logger with fields.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
Pull Request Description
RIC-T40 Build fix
This PR replaces the third-party
app-icon-badgeExpo plugin with a custom local implementation to resolve a build issue caused by a race condition in the original plugin.Changes
Replaced plugin reference in
app.config.ts: Switched from the externalapp-icon-badgeplugin to a custom./plugins/with-app-icon-badge.jswrapper.New custom plugin (
plugins/with-app-icon-badge.js): Collects all configured icons (main, iOS, and Android adaptive icon), passes them as batch jobs to a generator script, validates that each generated image was created successfully, and updates the Expo config to reference the new badged icons.New generation script (
scripts/generate-app-icon-badges.js): Wraps theaddBadgefunction from theapp-icon-badgelibrary with a workaround that addresses an async issue where image files weren't fully written before being used. The script uses a polling mechanism to verify PNG file integrity, writes to temporary files, and copies validated output to the final destination.Added unit tests (
plugins/__tests__/with-app-icon-badge.test.ts): Verifies that all configured icons are generated and config paths are updated when badges are enabled, and that icon paths remain untouched when badges are disabled.Purpose
The original
app-icon-badgeplugin had a bug where it did not awaitJimp.writeAsync, causing intermittent build failures when generated icon files were incomplete or missing. This custom implementation ensures reliable icon generation by properly waiting for valid output before proceeding.