Migrate kg-unsplash-selector to TypeScript#1762
Conversation
WalkthroughAdds new types (InsertImagePayload, InsertImageFn, SelectImgFn) and updates component and API signatures to use them; narrows multiple triggerDownload signatures to accept Pick<Photo, 'links'>. ESLint config changed to export an array of configurations, added languageOptions, explicit plugin mappings, expanded ignore/match patterns, and a test-specific config. package.json lint script and devDependencies updated. tsconfig.json migrated to ESNext/moduleResolution: bundler, jsx: react-jsx, enabled declarations/sourceMap, set noEmit, and broadened include to src/**/*. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
kevinansfield
force-pushed
the
ts/kg-unsplash-selector
branch
from
a71db75 to
ca27bfa
Compare
kevinansfield
force-pushed
the
ts/kg-unsplash-selector
branch
from
ca27bfa to
bad76f7
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: 2
🧹 Nitpick comments (2)
packages/kg-unsplash-selector/eslint.config.js (1)
71-71: Consider including TSX test files in the test override glob.If you have (or plan to add) React component tests,
test/**/*.tsxcurrently won’t receive the test-specific rule set.Suggested tweak
- files: ['test/**/*.ts'], + files: ['test/**/*.{ts,tsx}'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/eslint.config.js` at line 71, The override for test files currently only matches TypeScript files via the files: ['test/**/*.ts'] entry, so .tsx React test files won't get the test-specific rules; update the test override (the files array in the ESLint config where files: ['test/**/*.ts'] is declared) to also include 'test/**/*.tsx' (or a combined glob like 'test/**/*.{ts,tsx}') so TSX tests receive the same test-specific rule set.packages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.ts (1)
39-41: Normalizetermonce before filtering.This avoids repeated
toLowerCase()calls per item and keeps the predicate cleaner.♻️ Suggested refactor
public async searchPhotos(term: string): Promise<Photo[]> { this.SEARCH_IS_RUNNING = true; - const filteredPhotos = this.photos.filter(photo => (photo.description && photo.description.toLowerCase().includes(term.toLowerCase())) || - (photo.alt_description && photo.alt_description.toLowerCase().includes(term.toLowerCase())) + const normalizedTerm = term.toLowerCase(); + const filteredPhotos = this.photos.filter(photo => (photo.description && photo.description.toLowerCase().includes(normalizedTerm)) || + (photo.alt_description && photo.alt_description.toLowerCase().includes(normalizedTerm)) ); this.SEARCH_IS_RUNNING = false; return filteredPhotos; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.ts` around lines 39 - 41, In InMemoryUnsplashProvider, avoid calling term.toLowerCase() repeatedly inside the photos.filter predicate: compute a lowercased variable (e.g., const lowerTerm = term.toLowerCase()) before building filteredPhotos and then use lowerTerm in the predicate for description and alt_description checks (references: filteredPhotos, this.photos, term) so the predicate is cleaner and more efficient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-unsplash-selector/package.json`:
- Around line 36-37: The package.json contains incompatible dependency pairs:
bump eslint-plugin-react-hooks to 5.0.0+ to add ESLint v9 support (replace the
current eslint-plugin-react-hooks version constraint with ^5.0.0 or later so it
is compatible with eslint@9.37.0), and resolve the vitest/vite mismatch by
either upgrading vite to v6+ (set "vite" to ^6.0.0+) to satisfy vitest@4.1.0 or
downgrading vitest to a 3.x line (set "vitest" to ^3.0.0) to remain on vite@5.x;
update the package.json dependency entries for "eslint-plugin-react-hooks",
"vitest" and/or "vite" accordingly and run install to regenerate the lockfile.
In `@packages/kg-unsplash-selector/src/api/PhotoUseCase.ts`:
- Around line 19-20: The PhotoUseCase.triggerDownload currently calls
this._provider.triggerDownload(photo) without awaiting or returning it, which
lets provider rejections go unhandled; update PhotoUseCase.triggerDownload to
await the provider call (i.e., await this._provider.triggerDownload(photo)) so
both synchronous and async implementations are handled. Also update
UnsplashService.triggerDownload to either return the provider promise (propagate
it) or change its signature from void to Promise<void> so the upstream await
works correctly; ensure the provider interface/type reflects Promise<void> |
void if needed and adjust UnsplashService.triggerDownload to return the
Promise<void> path.
---
Nitpick comments:
In `@packages/kg-unsplash-selector/eslint.config.js`:
- Line 71: The override for test files currently only matches TypeScript files
via the files: ['test/**/*.ts'] entry, so .tsx React test files won't get the
test-specific rules; update the test override (the files array in the ESLint
config where files: ['test/**/*.ts'] is declared) to also include
'test/**/*.tsx' (or a combined glob like 'test/**/*.{ts,tsx}') so TSX tests
receive the same test-specific rule set.
In `@packages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.ts`:
- Around line 39-41: In InMemoryUnsplashProvider, avoid calling
term.toLowerCase() repeatedly inside the photos.filter predicate: compute a
lowercased variable (e.g., const lowerTerm = term.toLowerCase()) before building
filteredPhotos and then use lowerTerm in the predicate for description and
alt_description checks (references: filteredPhotos, this.photos, term) so the
predicate is cleaner and more efficient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4adfb698-50af-4f62-aa21-72f0b22b409c
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-unsplash-selector/eslint.config.jspackages/kg-unsplash-selector/package.jsonpackages/kg-unsplash-selector/src/UnsplashSearchModal.tsxpackages/kg-unsplash-selector/src/UnsplashTypes.tspackages/kg-unsplash-selector/src/api/IUnsplashProvider.tspackages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.tspackages/kg-unsplash-selector/src/api/PhotoUseCase.tspackages/kg-unsplash-selector/src/api/UnsplashProvider.tspackages/kg-unsplash-selector/src/api/UnsplashService.tspackages/kg-unsplash-selector/src/ui/UnsplashGallery.tsxpackages/kg-unsplash-selector/src/ui/UnsplashImage.tsxpackages/kg-unsplash-selector/src/ui/UnsplashZoomed.tsxpackages/kg-unsplash-selector/tsconfig.json
| "@eslint/js": "9.37.0", | ||
| "@playwright/test": "^1.48.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json
import urllib.parse
import urllib.request
packages = [
("eslint", "9.37.0"),
("typescript-eslint", "8.56.1"),
("@eslint/js", "9.37.0"),
("vite", "5.4.21"),
("@vitejs/plugin-react", "5.2.0"),
("vitest", "4.1.0"),
("vite-plugin-dts", "3.9.1"),
("@playwright/test", "1.48.2"),
("eslint-plugin-react-hooks", "4.6.2"),
]
def fetch_pkg(name, version):
escaped = urllib.parse.quote(name, safe="")
url = f"https://registry.npmjs.org/{escaped}/{version}"
with urllib.request.urlopen(url) as r:
return json.load(r)
for name, version in packages:
data = fetch_pkg(name, version)
peers = data.get("peerDependencies", {})
engines = data.get("engines", {})
print(f"\n{name}@{version}")
print(" peerDependencies:", json.dumps(peers, indent=2) if peers else "{}")
print(" engines:", json.dumps(engines, indent=2) if engines else "{}")
PYRepository: TryGhost/Koenig
Length of output: 2681
🏁 Script executed:
cat packages/kg-unsplash-selector/package.jsonRepository: TryGhost/Koenig
Length of output: 1818
🌐 Web query:
eslint 9.37.0 typescript-eslint 8.56.1 vite 5.4.21 compatibility
💡 Result:
Yes—ESLint 9.37.0 is compatible with typescript-eslint 8.56.1.
- typescript-eslint v8 supports ESLint
^8.57.0 || ^9.0.0 || ^10.0.0, so ESLint 9.37.0 is in-range. [1] (Also stated as “full support for ESLint v9” in the v8 announcement.) [2] - Vite 5.4.21 doesn’t directly constrain ESLint/typescript-eslint versions; its main relevant constraint is Node.js
^18.0.0 || >=20.0.0. [3]
If you hit issues, they’re usually from other ESLint plugins/configs in your stack not yet supporting ESLint 9 (not from typescript-eslint v8 itself). [2]
Sources: [1] https://typescript-eslint.io/users/dependency-versions/ [2] https://typescript-eslint.io/blog/announcing-typescript-eslint-v8/ [3] https://raw.githubusercontent.com/vitejs/vite/v5.4.21/packages/vite/package.json
🌐 Web query:
eslint-plugin-react-hooks 4.6.2 ESLint 9 compatibility
💡 Result:
eslint-plugin-react-hooks@4.6.2 is not ESLint 9–compatible in the officially supported sense, because its peerDependencies only allow ESLint up to v8 (^3 … ^8.0.0-0), so package managers will flag/deny installs with eslint@9. [1]
For ESLint 9, upgrade eslint-plugin-react-hooks to a version that adds ESLint 9 support—React published eslint-plugin-react-hooks@5.0.0 specifically to support ESLint v9 (and downstream tooling like Next.js unblocked their ESLint 9 upgrade based on that). [2]
Workaround if you must stay on 4.6.2: you can force-install with flags like --legacy-peer-deps, but that’s outside supported compatibility and may break. [3]
Sources: [1] (github.com) [2] (app.semanticdiff.com) [3] (github.com)
Citations:
- 1: cannot upgrade
eslintto v9 due toeslint-plugin-react-hookspeer dependancies facebook/react#30932 - 2: https://app.semanticdiff.com/gh/vercel/next.js/commit/300bfe52f4dd4fbf9b7e6aa7f9c832ce10ea9f41?utm_source=openai
- 3: Bug: [eslint-plugin-react-hooks] Compatibility issue with ESLint 9.x.x facebook/react#31158
🌐 Web query:
vitest 4.1.0 vite 5.4.21 compatibility
💡 Result:
Vitest 4.1.0 (released Mar 12, 2026) requires Vite >= 6.0.0 (and Node >= 20.0.0), so it’s not compatible with Vite 5.4.21. [1]
What to do:
- Upgrade Vite to v6+ to use Vitest 4.1.0. [1]
- Or stay on Vitest 3.x if you must remain on Vite 5.x (Vitest 3-era packages list Vite 5 as supported in their peer ranges). [2]
Sources:
[1] Vitest repo README / release info (Vitest requires Vite >=6; v4.1.0 release date) (github.com)
[2] Vitest discussion showing Vite 5 in supported peer range for Vitest 3-era packages (github.com)
Citations:
- 1: https://github.com/vitest-dev/vitest
- 2: installing only Vitest is ending up installing Vite@5.x by default with lots of CVE vitest-dev/vitest#7520
Multiple critical version incompatibilities found—these must be resolved before merge.
Verification reveals hard compatibility failures:
eslint-plugin-react-hooks@4.6.2is incompatible witheslint@9.37.0: The plugin's peerDependencies constraint is^3…^8.0.0-0(ESLint v8 only). Upgrade toeslint-plugin-react-hooks@5.0.0+, which adds ESLint v9 support.vitest@4.1.0is incompatible withvite@5.4.21: Vitest 4.1.0 requiresvite>=6.0.0. Either upgrade Vite to v6+ or downgrade Vitest to 3.x (which supports Vite 5.x).
These are not advisory issues—package managers will reject the lock with these combinations unless forced with unsupported flags like --legacy-peer-deps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-unsplash-selector/package.json` around lines 36 - 37, The
package.json contains incompatible dependency pairs: bump
eslint-plugin-react-hooks to 5.0.0+ to add ESLint v9 support (replace the
current eslint-plugin-react-hooks version constraint with ^5.0.0 or later so it
is compatible with eslint@9.37.0), and resolve the vitest/vite mismatch by
either upgrading vite to v6+ (set "vite" to ^6.0.0+) to satisfy vitest@4.1.0 or
downgrading vitest to a 3.x line (set "vitest" to ^3.0.0) to remain on vite@5.x;
update the package.json dependency entries for "eslint-plugin-react-hooks",
"vitest" and/or "vite" accordingly and run install to regenerate the lockfile.
| async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> { | ||
| this._provider.triggerDownload(photo); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify triggerDownload chaining/handling across layers
rg -nP --type=ts -C3 '\btriggerDownload\s*\(' \
packages/kg-unsplash-selector/src/api/IUnsplashProvider.ts \
packages/kg-unsplash-selector/src/api/PhotoUseCase.ts \
packages/kg-unsplash-selector/src/api/UnsplashService.ts \
packages/kg-unsplash-selector/src/UnsplashSearchModal.tsxRepository: TryGhost/Koenig
Length of output: 3273
triggerDownload drops provider failures due to missing await.
On line 20, this._provider.triggerDownload(photo) is invoked inside an async method but not awaited or returned. If the provider returns a rejecting promise, it will escape unhandled. Since the provider interface declares the return type as Promise<void> | void, use await to handle both synchronous and asynchronous cases.
Additionally, packages/kg-unsplash-selector/src/api/UnsplashService.ts needs attention: its triggerDownload method (lines 64-66) should either propagate the promise by returning it, or change the interface signature from void to Promise<void> to reflect the actual behavior after fixing PhotoUseCase.
✅ Proposed fix (PhotoUseCase.ts)
- async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> {
- this._provider.triggerDownload(photo);
- }
+ async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> {
+ await this._provider.triggerDownload(photo);
+ }📝 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.
| async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> { | |
| this._provider.triggerDownload(photo); | |
| async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> { | |
| await this._provider.triggerDownload(photo); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-unsplash-selector/src/api/PhotoUseCase.ts` around lines 19 - 20,
The PhotoUseCase.triggerDownload currently calls
this._provider.triggerDownload(photo) without awaiting or returning it, which
lets provider rejections go unhandled; update PhotoUseCase.triggerDownload to
await the provider call (i.e., await this._provider.triggerDownload(photo)) so
both synchronous and async implementations are handled. Also update
UnsplashService.triggerDownload to either return the provider promise (propagate
it) or change its signature from void to Promise<void> so the upstream await
works correctly; ensure the provider interface/type reflects Promise<void> |
void if needed and adjust UnsplashService.triggerDownload to return the
Promise<void> path.
6050b29 to
2913b5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kg-unsplash-selector/eslint.config.js (2)
71-71: Patterntest/**/*.tsis safe, but consider expanding to.tsxfor future-proofing.No TSX test files currently exist in the test directory. However, if you plan to add TSX tests in the future, expanding the pattern to
test/**/*.{ts,tsx}would ensure consistent rule application.Suggested patch
- files: ['test/**/*.ts'], + files: ['test/**/*.{ts,tsx}'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/eslint.config.js` at line 71, Update the ESLint file pattern that currently lists "test/**/*.ts" so it also matches TSX test files for future-proofing; replace the array entry files: ['test/**/*.ts'] (in the ESLint config) with files: ['test/**/*.{ts,tsx}'] to ensure rules apply to both .ts and .tsx test files.
9-10: Migrate from deprecatedtseslint.config(...)todefineConfig(...).Line 9 uses
tseslint.config(...), which is now deprecated intypescript-eslintin favor of ESLint core'sdefineConfig(...). Your repo's ESLint version (9.37.0) supportsdefineConfigfromeslint/config, so this migration is straightforward:- import tseslint from 'typescript-eslint'; + import { defineConfig } from 'eslint/config'; + import tseslint from 'typescript-eslint'; - export default tseslint.config([ + export default defineConfig([🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/eslint.config.js` around lines 9 - 10, The file uses the deprecated tseslint.config(...) export; replace it with ESLint core's defineConfig(...) from 'eslint/config' — import { defineConfig } from 'eslint/config' (instead of using tseslint.config) and wrap the same configuration array/object in defineConfig(...), keeping the existing ignores and other rules intact; update any related import names (remove/replace tseslint references) so export default defineConfig({...}) is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-unsplash-selector/tsconfig.json`:
- Around line 9-17: Remove the inert emit-related compiler options from
tsconfig.json: delete the "declaration", "declarationMap", and "sourceMap"
properties since "noEmit" is true and declaration generation is handled by
vite-plugin-dts; keep "noEmit" and the other type-checking flags intact to avoid
confusion.
---
Nitpick comments:
In `@packages/kg-unsplash-selector/eslint.config.js`:
- Line 71: Update the ESLint file pattern that currently lists "test/**/*.ts" so
it also matches TSX test files for future-proofing; replace the array entry
files: ['test/**/*.ts'] (in the ESLint config) with files:
['test/**/*.{ts,tsx}'] to ensure rules apply to both .ts and .tsx test files.
- Around line 9-10: The file uses the deprecated tseslint.config(...) export;
replace it with ESLint core's defineConfig(...) from 'eslint/config' — import {
defineConfig } from 'eslint/config' (instead of using tseslint.config) and wrap
the same configuration array/object in defineConfig(...), keeping the existing
ignores and other rules intact; update any related import names (remove/replace
tseslint references) so export default defineConfig({...}) is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 282ae63d-e5f1-41f2-a6cb-606f0e229792
📒 Files selected for processing (13)
packages/kg-unsplash-selector/eslint.config.jspackages/kg-unsplash-selector/package.jsonpackages/kg-unsplash-selector/src/UnsplashSearchModal.tsxpackages/kg-unsplash-selector/src/UnsplashTypes.tspackages/kg-unsplash-selector/src/api/IUnsplashProvider.tspackages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.tspackages/kg-unsplash-selector/src/api/PhotoUseCase.tspackages/kg-unsplash-selector/src/api/UnsplashProvider.tspackages/kg-unsplash-selector/src/api/UnsplashService.tspackages/kg-unsplash-selector/src/ui/UnsplashGallery.tsxpackages/kg-unsplash-selector/src/ui/UnsplashImage.tsxpackages/kg-unsplash-selector/src/ui/UnsplashZoomed.tsxpackages/kg-unsplash-selector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/kg-unsplash-selector/src/api/IUnsplashProvider.ts
- packages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.ts
- packages/kg-unsplash-selector/src/api/PhotoUseCase.ts
- packages/kg-unsplash-selector/src/ui/UnsplashGallery.tsx
- packages/kg-unsplash-selector/src/api/UnsplashService.ts
- packages/kg-unsplash-selector/package.json
| "declaration": true, | ||
| "declarationMap": true, | ||
| "sourceMap": true, | ||
| "esModuleInterop": true, | ||
| "forceConsistentCasingInFileNames": true, | ||
| "module": "ESNext", | ||
| "moduleResolution": "Node", | ||
| "skipLibCheck": true, | ||
| "resolveJsonModule": true, | ||
| "isolatedModules": true, | ||
| "jsx": "react-jsx", | ||
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "declarationMap": true, | ||
| "declaration": true, | ||
| "emitDeclarationOnly": true, | ||
| "declarationDir": "./types", | ||
| "baseUrl": ".", | ||
| "paths": { | ||
| "kg-unsplash-selector": ["src/index.ts"], | ||
| }, | ||
| "typeRoots": ["node_modules/@types", "types/index.d.ts"], | ||
| "noEmit": true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this package actually needs emitted declaration files.
# Expected:
# - If package publishes types from build output, keep declaration flags in a build-specific tsconfig.
# - If package is type-check only, declaration flags can be removed from this file.
set -euo pipefail
echo "== tsconfig compilerOptions =="
jq '.compilerOptions | {noEmit, declaration, declarationMap, sourceMap}' packages/kg-unsplash-selector/tsconfig.json
echo
echo "== package metadata that may require emitted types =="
jq '{name, types, typings, files, exports}' packages/kg-unsplash-selector/package.jsonRepository: TryGhost/Koenig
Length of output: 412
🏁 Script executed:
#!/bin/bash
# Search for all tsconfig files in the package
echo "=== tsconfig files in kg-unsplash-selector package ==="
find packages/kg-unsplash-selector -name "tsconfig*.json" -type f
echo
echo "=== package.json build scripts ==="
jq '.scripts' packages/kg-unsplash-selector/package.json
echo
echo "=== Check if dist directory exists ==="
ls -la packages/kg-unsplash-selector/ | grep -E "^d|^-"Repository: TryGhost/Koenig
Length of output: 2035
🏁 Script executed:
#!/bin/bash
echo "=== tsconfig.declaration.json ==="
cat packages/kg-unsplash-selector/tsconfig.declaration.json
echo
echo "=== tsconfig.node.json ==="
cat packages/kg-unsplash-selector/tsconfig.node.json
echo
echo "=== Main tsconfig.json (full file) ==="
cat packages/kg-unsplash-selector/tsconfig.jsonRepository: TryGhost/Koenig
Length of output: 1126
🏁 Script executed:
#!/bin/bash
echo "=== vite.config.ts (check output paths) ==="
cat packages/kg-unsplash-selector/vite.config.ts
echo
echo "=== Check if 'types' output handling in package.json ==="
head -20 packages/kg-unsplash-selector/package.jsonRepository: TryGhost/Koenig
Length of output: 1716
Remove inert emit-related flags from tsconfig.json.
The flags "declaration", "declarationMap", and "sourceMap" on lines 9–11 have no effect because "noEmit": true (line 17) prevents TypeScript from emitting any files. Declaration generation is handled by vite-plugin-dts during the vite build step, so these flags should be removed from the main tsconfig (which is used for type-checking only).
You can safely delete lines 9–11 to reduce configuration noise and prevent future confusion.
Cleanup
"strict": true,
- "declaration": true,
- "declarationMap": true,
- "sourceMap": true,
"esModuleInterop": true,📝 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.
| "declaration": true, | |
| "declarationMap": true, | |
| "sourceMap": true, | |
| "esModuleInterop": true, | |
| "forceConsistentCasingInFileNames": true, | |
| "module": "ESNext", | |
| "moduleResolution": "Node", | |
| "skipLibCheck": true, | |
| "resolveJsonModule": true, | |
| "isolatedModules": true, | |
| "jsx": "react-jsx", | |
| "skipLibCheck": true, | |
| "esModuleInterop": true, | |
| "declarationMap": true, | |
| "declaration": true, | |
| "emitDeclarationOnly": true, | |
| "declarationDir": "./types", | |
| "baseUrl": ".", | |
| "paths": { | |
| "kg-unsplash-selector": ["src/index.ts"], | |
| }, | |
| "typeRoots": ["node_modules/@types", "types/index.d.ts"], | |
| "noEmit": true | |
| "esModuleInterop": true, | |
| "forceConsistentCasingInFileNames": true, | |
| "skipLibCheck": true, | |
| "resolveJsonModule": true, | |
| "isolatedModules": true, | |
| "noEmit": true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-unsplash-selector/tsconfig.json` around lines 9 - 17, Remove the
inert emit-related compiler options from tsconfig.json: delete the
"declaration", "declarationMap", and "sourceMap" properties since "noEmit" is
true and declaration generation is handled by vite-plugin-dts; keep "noEmit" and
the other type-checking flags intact to avoid confusion.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/kg-unsplash-selector/src/api/PhotoUseCase.ts (1)
19-21:⚠️ Potential issue | 🟠 MajorMissing
awaiton provider call.The
asyncmethod declaresPromise<void>but doesn't await the provider call, causing rejected promises to go unhandled.Proposed fix
async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> { - this._provider.triggerDownload(photo); + await this._provider.triggerDownload(photo); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/src/api/PhotoUseCase.ts` around lines 19 - 21, The async method triggerDownload in PhotoUseCase calls this._provider.triggerDownload(photo) without awaiting it, so rejections may be unhandled; modify triggerDownload (the method in PhotoUseCase) to await the provider call (or return the provider promise) by changing the call to await this._provider.triggerDownload(photo) so the Promise<void> correctly represents completion and errors propagate to callers.
🧹 Nitpick comments (2)
packages/kg-unsplash-selector/eslint.config.js (2)
71-71: Consider including TSX tests in the test override glob.Line 71 only matches
test/**/*.ts; if TSX tests are introduced, these test-specific rule overrides won’t apply.🧩 Suggested tweak
- files: ['test/**/*.ts'], + files: ['test/**/*.{ts,tsx}'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/eslint.config.js` at line 71, The test override currently only targets 'test/**/*.ts' so TSX test files won't get the same ESLint overrides; update the override glob referenced by files (the array entry 'test/**/*.ts') to also match TSX files (e.g., use a brace pattern or add 'test/**/*.tsx') so test-specific rules apply to .tsx tests as well.
9-10: PreferdefineConfigover deprecatedtseslint.confighelper.Line 9 currently uses
tseslint.config(...), which is deprecated in typescript-eslint v8.x. Migrate todefineConfigfromeslint/configto align with upstream guidance and avoid future maintenance churn.♻️ Proposed refactor
import eslint from '@eslint/js'; +import { defineConfig } from 'eslint/config'; import ghostPlugin from 'eslint-plugin-ghost'; import tseslint from 'typescript-eslint'; import reactPlugin from 'eslint-plugin-react'; import reactHooksPlugin from 'eslint-plugin-react-hooks'; import reactRefreshPlugin from 'eslint-plugin-react-refresh'; import tailwindPlugin from 'eslint-plugin-tailwindcss'; -export default tseslint.config([ +export default defineConfig([ {ignores: ['dist/**', 'types/**']}, { files: ['**/*.{ts,tsx}'], extends: [ eslint.configs.recommended, tseslint.configs.recommended ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/eslint.config.js` around lines 9 - 10, Replace the deprecated tseslint.config usage with defineConfig from 'eslint/config': import defineConfig from 'eslint/config' (or destructure it) at the top, then change the export default line from export default tseslint.config([...]) to export default defineConfig([...]) while keeping the existing configuration object (e.g., the ignores array and any other entries) and file patterns intact; update any references to tseslint.config in this file to defineConfig to complete the migration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/kg-unsplash-selector/src/api/PhotoUseCase.ts`:
- Around line 19-21: The async method triggerDownload in PhotoUseCase calls
this._provider.triggerDownload(photo) without awaiting it, so rejections may be
unhandled; modify triggerDownload (the method in PhotoUseCase) to await the
provider call (or return the provider promise) by changing the call to await
this._provider.triggerDownload(photo) so the Promise<void> correctly represents
completion and errors propagate to callers.
---
Nitpick comments:
In `@packages/kg-unsplash-selector/eslint.config.js`:
- Line 71: The test override currently only targets 'test/**/*.ts' so TSX test
files won't get the same ESLint overrides; update the override glob referenced
by files (the array entry 'test/**/*.ts') to also match TSX files (e.g., use a
brace pattern or add 'test/**/*.tsx') so test-specific rules apply to .tsx tests
as well.
- Around line 9-10: Replace the deprecated tseslint.config usage with
defineConfig from 'eslint/config': import defineConfig from 'eslint/config' (or
destructure it) at the top, then change the export default line from export
default tseslint.config([...]) to export default defineConfig([...]) while
keeping the existing configuration object (e.g., the ignores array and any other
entries) and file patterns intact; update any references to tseslint.config in
this file to defineConfig to complete the migration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bee0bb8-8ffd-4d76-ada6-c63ebb4e8691
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-unsplash-selector/eslint.config.jspackages/kg-unsplash-selector/package.jsonpackages/kg-unsplash-selector/src/UnsplashSearchModal.tsxpackages/kg-unsplash-selector/src/UnsplashTypes.tspackages/kg-unsplash-selector/src/api/IUnsplashProvider.tspackages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.tspackages/kg-unsplash-selector/src/api/PhotoUseCase.tspackages/kg-unsplash-selector/src/api/UnsplashProvider.tspackages/kg-unsplash-selector/src/api/UnsplashService.tspackages/kg-unsplash-selector/src/ui/UnsplashGallery.tsxpackages/kg-unsplash-selector/src/ui/UnsplashImage.tsxpackages/kg-unsplash-selector/src/ui/UnsplashZoomed.tsxpackages/kg-unsplash-selector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/kg-unsplash-selector/src/ui/UnsplashImage.tsx
- packages/kg-unsplash-selector/src/UnsplashSearchModal.tsx
- packages/kg-unsplash-selector/src/UnsplashTypes.ts
- packages/kg-unsplash-selector/package.json
- packages/kg-unsplash-selector/src/api/UnsplashService.ts
kevinansfield
force-pushed
the
ts/kg-unsplash-selector
branch
from
2913b5f to
3d1a04e
Compare
- Update tsconfig.json: bundler moduleResolution, modern minimal config - Replace .eslintrc.cjs with eslint.config.js (flat config) - Switch to unified typescript-eslint package
kevinansfield
force-pushed
the
ts/kg-unsplash-selector
branch
from
3d1a04e to
3655205
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/kg-unsplash-selector/eslint.config.js (1)
71-71: Consider including TSX test files in the test override glob.Line 71 currently matches only
test/**/*.ts; if this package adds component-style tests (.tsx), those Mocha overrides won’t apply.Suggested adjustment
- files: ['test/**/*.ts'], + files: ['test/**/*.{ts,tsx}'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/kg-unsplash-selector/eslint.config.js` at line 71, Update the ESLint override that sets files: ['test/**/*.ts'] to also match TSX tests by expanding the glob (e.g., include 'test/**/*.tsx' or use a pattern like 'test/**/*.{ts,tsx}') so the Mocha/test-specific overrides apply to component-style tests; locate the override object containing the files property in packages/kg-unsplash-selector/eslint.config.js and change the files array accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/kg-unsplash-selector/src/api/UnsplashService.ts`:
- Line 12: The interface method triggerDownload currently declares a void return
but calls photoUseCases.triggerDownload(photo) which returns Promise<void>,
causing unhandled rejections; change the method signature from void to
Promise<void> and update its implementation (e.g., make triggerDownload async or
return the promise) so it either awaits or returns
photoUseCases.triggerDownload(photo); apply the same change for the other
occurrences referenced (lines 64–66) so all declarations/implementations
consistently return Promise<void>.
---
Nitpick comments:
In `@packages/kg-unsplash-selector/eslint.config.js`:
- Line 71: Update the ESLint override that sets files: ['test/**/*.ts'] to also
match TSX tests by expanding the glob (e.g., include 'test/**/*.tsx' or use a
pattern like 'test/**/*.{ts,tsx}') so the Mocha/test-specific overrides apply to
component-style tests; locate the override object containing the files property
in packages/kg-unsplash-selector/eslint.config.js and change the files array
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 151c9587-9b77-4020-8cf0-666cb0a3a085
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
packages/kg-unsplash-selector/eslint.config.jspackages/kg-unsplash-selector/package.jsonpackages/kg-unsplash-selector/src/UnsplashSearchModal.tsxpackages/kg-unsplash-selector/src/UnsplashTypes.tspackages/kg-unsplash-selector/src/api/IUnsplashProvider.tspackages/kg-unsplash-selector/src/api/InMemoryUnsplashProvider.tspackages/kg-unsplash-selector/src/api/PhotoUseCase.tspackages/kg-unsplash-selector/src/api/UnsplashProvider.tspackages/kg-unsplash-selector/src/api/UnsplashService.tspackages/kg-unsplash-selector/src/ui/UnsplashGallery.tsxpackages/kg-unsplash-selector/src/ui/UnsplashImage.tsxpackages/kg-unsplash-selector/src/ui/UnsplashZoomed.tsxpackages/kg-unsplash-selector/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/kg-unsplash-selector/src/ui/UnsplashZoomed.tsx
- packages/kg-unsplash-selector/src/UnsplashTypes.ts
- packages/kg-unsplash-selector/src/api/IUnsplashProvider.ts
- packages/kg-unsplash-selector/package.json
| loadNextPage(): Promise<void>; | ||
| clearPhotos(): void; | ||
| triggerDownload(photo: Photo): void; | ||
| triggerDownload(photo: Pick<Photo, 'links'>): void; |
There was a problem hiding this comment.
Interface declares void but delegates to async method without awaiting.
The triggerDownload method calls this.photoUseCases.triggerDownload(photo) which returns Promise<void>, but neither awaits nor returns it. If the underlying provider rejects, the error will be unhandled.
Consider making this method async and returning the promise:
🔧 Proposed fix
export interface IUnsplashService {
// ...
- triggerDownload(photo: Pick<Photo, 'links'>): void;
+ triggerDownload(photo: Pick<Photo, 'links'>): Promise<void>;
// ...
}- triggerDownload(photo: Pick<Photo, 'links'>) {
- this.photoUseCases.triggerDownload(photo);
+ async triggerDownload(photo: Pick<Photo, 'links'>): Promise<void> {
+ await this.photoUseCases.triggerDownload(photo);
}Also applies to: 64-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/kg-unsplash-selector/src/api/UnsplashService.ts` at line 12, The
interface method triggerDownload currently declares a void return but calls
photoUseCases.triggerDownload(photo) which returns Promise<void>, causing
unhandled rejections; change the method signature from void to Promise<void> and
update its implementation (e.g., make triggerDownload async or return the
promise) so it either awaits or returns photoUseCases.triggerDownload(photo);
apply the same change for the other occurrences referenced (lines 64–66) so all
declarations/implementations consistently return Promise<void>.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1762 +/- ##
==========================================
+ Coverage 88.51% 88.58% +0.06%
==========================================
Files 201 200 -1
Lines 18456 18454 -2
Branches 2019 2021 +2
==========================================
+ Hits 16337 16348 +11
+ Misses 2108 2095 -13
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
Note: kg-unsplash-selector was already TypeScript. This PR updates the tooling configuration to align with the monorepo-wide TypeScript migration.
Test plan
yarn lintpasses in kg-unsplash-selector