Repository navigation
Fixes from the case-study review - #25
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| @@ -0,0 +1,8 @@ | |||
| export const readStoredArray = (key: string): string[] => { | |||
There was a problem hiding this comment.
WARNING: Return type string[] doesn't match runtime behavior\n\nThe function returns parsed directly when it's an array, but JSON.parse can produce arrays of any type (numbers, objects, mixed). The TypeScript signature claims string[] but runtime values could be number[], object[], etc. Callers in App.tsx and OnboardingContext.tsx assume string arrays.\n\nConsider either:\n1. Validate element types: return Array.isArray(parsed) ? parsed.filter(v => typeof v === 'string') : []\n2. Widen return type to unknown[] and add runtime checks at call sites\n3. Keep as-is but change return type to Array<string | number | boolean | object | null> for accuracy
| let lastNodeType = ''; | ||
| const formatPrelude = (prelude: string): string => { | ||
| const text = squash(prelude); | ||
| if (text.startsWith('@')) return text.replace(/\(\s*([\w-]+)\s*:\s*/g, '($1: '); |
There was a problem hiding this comment.
WARNING: formatPrelude regex only handles word: patterns in parentheses\n\nThe regex /\\(\\s*([\\w-]+)\\s*:\\s*/g matches patterns like (min-width: 600px) but doesn't handle complex media query preludes such as @media not all and (min-width: 600px) where not all and appears outside parentheses. The non-parenthesized portion is preserved as-is (which works), but the regex is narrowly scoped.\n\nTest cases cover @media, @supports selector(), and @page but not complex queries like not, only, or multiple comma-separated media conditions.
| buffer += ch; | ||
| if (ch === '\\') buffer += css[++i] ?? ''; | ||
| else if (ch === quote) quote = ''; | ||
| } else if (ch === '\\') { |
There was a problem hiding this comment.
SUGGESTION: Escape handling modifies loop index i directly\n\nUsing css[++i] inside the for loop works but is error-prone and hard to read. Consider:\n\n} else if (ch === '\\') {\n const nextChar = css[i + 1] ?? '';\n buffer += ch + nextChar;\n i++;\n}\n\nExplicitly accessing css[i + 1] and incrementing i once makes the control flow clearer and avoids confusion with the loop's own i++.
| const savedOrder = localStorage.getItem("toolsOrder"); | ||
| if (savedOrder) { | ||
| const orderIds = JSON.parse(savedOrder); | ||
| const orderIds = readStoredArray("toolsOrder"); |
There was a problem hiding this comment.
SUGGESTION: readStoredArray return type mismatch\n\nreadStoredArray is typed as returning string[] but can return arrays of other types at runtime. This line assumes string elements for .map((id: string) => ...). If localStorage contains [1, 2, 3], TypeScript won't catch the mismatch but runtime behavior may differ.\n\nAdd a runtime guard: const orderIds = readStoredArray("toolsOrder").filter(v => typeof v === 'string') or update readStoredArray to validate element types.
| const hasSeenWelcome = !!localStorage.getItem(STORAGE_KEYS.WELCOME_SEEN); | ||
| const hasCompletedTour = !!localStorage.getItem(STORAGE_KEYS.TOUR_COMPLETED); | ||
| const tourProgress = JSON.parse(localStorage.getItem(STORAGE_KEYS.TOUR_PROGRESS) || "[]"); | ||
| const tourProgress = readStoredArray(STORAGE_KEYS.TOUR_PROGRESS); |
There was a problem hiding this comment.
SUGGESTION: readStoredArray return type mismatch\n\ntourProgress is typed as string[] in OnboardingState but readStoredArray can return non-string arrays. Same concern as in App.tsx.\n\nConsider: const tourProgress = readStoredArray(STORAGE_KEYS.TOUR_PROGRESS).filter(v => typeof v === 'string')
|
|
||
| const markFeatureDiscovered = (feature: string) => { | ||
| const discoveries = JSON.parse(localStorage.getItem(STORAGE_KEYS.FEATURE_DISCOVERIES) || "[]"); | ||
| const discoveries = readStoredArray(STORAGE_KEYS.FEATURE_DISCOVERIES); |
There was a problem hiding this comment.
SUGGESTION: readStoredArray return type mismatch\n\ndiscoveries used with .includes(feature) where feature is a string. If the stored array contains non-strings, includes may behave unexpectedly.\n\nAdd filter: const discoveries = readStoredArray(STORAGE_KEYS.FEATURE_DISCOVERIES).filter(v => typeof v === 'string')
|
|
||
| const isFeatureDiscovered = (feature: string): boolean => { | ||
| const discoveries = JSON.parse(localStorage.getItem(STORAGE_KEYS.FEATURE_DISCOVERIES) || "[]"); | ||
| const discoveries = readStoredArray(STORAGE_KEYS.FEATURE_DISCOVERIES); |
There was a problem hiding this comment.
SUGGESTION: readStoredArray return type mismatch\n\nSame issue as line 118 - isFeatureDiscovered uses .includes(feature) on potentially non-string array.\n\nAdd filter: const discoveries = readStoredArray(STORAGE_KEYS.FEATURE_DISCOVERIES).filter(v => typeof v === 'string')
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (13 files)
Fix these issues in Kilo Cloud Reviewed by free · Input: 344.3K · Output: 21.3K · Cached: 1.4M |
This branch was successfully deployed
1 participant
Fixes found while writing the case study. One commit per fix: