Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
taskyliz
force-pushed
the
feat/versions-diff-view
branch
from
28c17c5 to
a787b16
Compare
taskyliz
force-pushed
the
feat/versions-diff-view
branch
from
a787b16 to
3149f5d
Compare
taskyliz
force-pushed
the
feat/versions-diff-view
branch
from
3149f5d to
376868d
Compare
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
I'll make some tweaks later - but this looks amazing!
a few notes to myself (or you if you get a chance!):
- we should have consistent font size for code trees between diff viewer + code viewer
- we should have a similar kind of mobile panel that slides in between the two of them
- we should use the toggles from settings.vue rather than checkboxes
- I think there are some hydration issues but not sure if that originates in this PR
- the sliders can also be more aligned with the existing visual styles - some nice work from @serhalp in the search filters might be a helpful guide
taskyliz
force-pushed
the
feat/versions-diff-view
branch
from
1259bc3 to
a02950c
Compare
taskyliz
force-pushed
the
feat/versions-diff-view
branch
from
181dd7f to
3619565
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: 10
🧹 Nitpick comments (3)
package.json (1)
56-56: Duplicate dependency@intlify/core-basein bothdependenciesanddevDependencies.This package appears at line 56 in
dependenciesand again at line 111 indevDependencieswith the same version11.2.8. This is redundant—packages independenciesare already available during development. Consider removing thedevDependenciesentry unless there's a specific reason for the duplication.app/utils/language-detection.ts (1)
64-72:filenameMapis case-sensitive—lowercase variants will fall through to extension lookup.Files named
dockerfileormakefile(lowercase, common in many projects) won't match the exact keysDockerfileandMakefile, and will fall back to extension-based lookup (returning'text'since they have no extension).Consider a case-insensitive lookup:
🔧 Proposed fix
- if (filenameMap[fileName]) { - return filenameMap[fileName] + const lowerFileName = fileName.toLowerCase() + const filenameMatch = Object.entries(filenameMap).find( + ([key]) => key.toLowerCase() === lowerFileName + ) + if (filenameMatch) { + return filenameMatch[1] }Alternatively, store lowercase keys in
filenameMapand compare againstfileName.toLowerCase().app/components/diff/Line.vue (1)
112-118: Consider ifdeep: trueis necessary for the watch.The watch uses
deep: truewhich recursively watchesprops.line. SinceDiffLineobjects are typically replaced rather than mutated, a shallow watch might suffice and be more performant. However, this is a minor optimisation consideration.
| }) | ||
| const borderClasses = computed(() => { | ||
| const classes = ['border-transparent', 'w-1', 'border-l-3'] |
There was a problem hiding this comment.
Use RTL-compatible utility class for border.
The pipeline flagged border-l-3 as incompatible with RTL layouts. Replace with border-is-3 for proper bidirectional support.
🔧 Proposed fix
- const classes = ['border-transparent', 'w-1', 'border-l-3']
+ const classes = ['border-transparent', 'w-1', 'border-is-3']🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 52-52: [RTL] avoid using 'border-l-3', use 'border-is-3' instead.
| const contentClasses = computed(() => { | ||
| const shouldWrap = diffContext?.wordWrap?.value ?? false | ||
| return ['pr-6', shouldWrap ? 'whitespace-pre-wrap break-words' : 'text-nowrap'] |
There was a problem hiding this comment.
Use RTL-compatible utility class for padding.
The pipeline flagged pr-6 as incompatible with RTL layouts. Replace with pe-6 for proper bidirectional support.
🔧 Proposed fix
- return ['pr-6', shouldWrap ? 'whitespace-pre-wrap break-words' : 'text-nowrap']
+ return ['pe-6', shouldWrap ? 'whitespace-pre-wrap break-words' : 'text-nowrap']📝 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.
| return ['pr-6', shouldWrap ? 'whitespace-pre-wrap break-words' : 'text-nowrap'] | |
| return ['pe-6', shouldWrap ? 'whitespace-pre-wrap break-words' : 'text-nowrap'] |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 66-66: [RTL] avoid using 'pr-6', use 'pe-6' instead.
| {{ section.label }} ({{ section.changes.length }}) | ||
| </summary> | ||
|
|
||
| <div class="space-y-1 ml-5 max-h-40 overflow-y-auto"> |
There was a problem hiding this comment.
Fix RTL compliance: replace ml-5 with ms-5.
The pipeline reports an RTL linting error. Use logical margin property ms-5 (margin-inline-start) instead of ml-5 for proper right-to-left language support.
🔧 Proposed fix
- <div class="space-y-1 ml-5 max-h-40 overflow-y-auto">
+ <div class="space-y-1 ms-5 max-h-40 overflow-y-auto">📝 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.
| <div class="space-y-1 ml-5 max-h-40 overflow-y-auto"> | |
| <div class="space-y-1 ms-5 max-h-40 overflow-y-auto"> |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 117-117: [RTL] avoid using 'ml-5', use 'ms-5' instead.
| </NuxtLink> | ||
|
|
||
| <div | ||
| class="flex items-center gap-1.5 text-fg-muted font-mono text-[10px] ml-auto shrink-0" |
There was a problem hiding this comment.
Fix RTL compliance: replace ml-auto with ms-auto.
The pipeline reports an RTL linting error. Use logical margin property ms-auto instead of ml-auto for proper right-to-left language support.
🔧 Proposed fix
- <div
- class="flex items-center gap-1.5 text-fg-muted font-mono text-[10px] ml-auto shrink-0"
+ <div
+ class="flex items-center gap-1.5 text-fg-muted font-mono text-[10px] ms-auto shrink-0"📝 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.
| class="flex items-center gap-1.5 text-fg-muted font-mono text-[10px] ml-auto shrink-0" | |
| class="flex items-center gap-1.5 text-fg-muted font-mono text-[10px] ms-auto shrink-0" |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 142-142: [RTL] avoid using 'ml-auto', use 'ms-auto' instead.
| <span class="i-lucide-chevrons-up-down w-4 h-4" /> | ||
| </td> | ||
| <td> | ||
| <span class="px-0 sticky left-2 italic opacity-50"> |
There was a problem hiding this comment.
Use RTL-compatible utility class for positioning.
The pipeline flagged left-2 as incompatible with RTL layouts. Replace with inset-is-2 for proper bidirectional support.
🔧 Proposed fix
<span class="px-0 sticky left-2 italic opacity-50">
+ <span class="px-0 sticky inset-is-2 italic opacity-50">🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 19-19: [RTL] avoid using 'left-2', use 'inset-is-2' instead.
| <!-- Dropdown menu --> | ||
| <motion.div | ||
| v-if="showOptions" | ||
| class="absolute right-0 top-full mt-2 z-20 p-4 bg-bg-elevated border border-border shadow-2xl overflow-auto" |
There was a problem hiding this comment.
Fix RTL compliance: replace right-0 with inset-ie-0.
The pipeline reports an RTL linting error. Use logical inset property inset-ie-0 (inset-inline-end) instead of right-0 for proper right-to-left language support.
🔧 Proposed fix
<motion.div
v-if="showOptions"
- class="absolute right-0 top-full mt-2 z-20 p-4 bg-bg-elevated border border-border shadow-2xl overflow-auto"
+ class="absolute inset-ie-0 top-full mt-2 z-20 p-4 bg-bg-elevated border border-border shadow-2xl overflow-auto"📝 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.
| class="absolute right-0 top-full mt-2 z-20 p-4 bg-bg-elevated border border-border shadow-2xl overflow-auto" | |
| <motion.div | |
| v-if="showOptions" | |
| class="absolute inset-ie-0 top-full mt-2 z-20 p-4 bg-bg-elevated border border-border shadow-2xl overflow-auto" |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 278-278: [RTL] avoid using 'right-0', use 'inset-ie-0' instead.
| <div v-else-if="compare" class="flex-1 flex flex-col md:flex-row min-h-0 overflow-hidden"> | ||
| <!-- Desktop sidebar --> | ||
| <aside | ||
| class="hidden md:flex w-80 border-r border-border bg-bg-subtle flex-col shrink-0 min-h-0" |
There was a problem hiding this comment.
Fix RTL compliance: replace border-r with border-ie.
The pipeline reports an RTL linting error. Use logical border property border-ie (border-inline-end) instead of border-r for proper right-to-left language support.
🔧 Proposed fix
<aside
- class="hidden md:flex w-80 border-r border-border bg-bg-subtle flex-col shrink-0 min-h-0"
+ class="hidden md:flex w-80 border-ie border-border bg-bg-subtle flex-col shrink-0 min-h-0"
>📝 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.
| class="hidden md:flex w-80 border-r border-border bg-bg-subtle flex-col shrink-0 min-h-0" | |
| <aside | |
| class="hidden md:flex w-80 border-ie border-border bg-bg-subtle flex-col shrink-0 min-h-0" | |
| > |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 204-204: [RTL] avoid using 'border-r', use 'border-ie' instead.
| "view_file": "View file", | ||
| "view_in_browser": "View in code browser", | ||
| "close_diff": "Close diff", | ||
| "lines_hidden": "{count} lines hidden", |
There was a problem hiding this comment.
Add pluralisation for lines_hidden.
The file already uses plural syntax for lines at line 608. This key should follow the same pattern to avoid rendering "1 lines hidden".
Proposed fix
- "lines_hidden": "{count} lines hidden",
+ "lines_hidden": "{count} line hidden | {count} lines hidden",📝 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.
| "lines_hidden": "{count} lines hidden", | |
| "lines_hidden": "{count} line hidden | {count} lines hidden", |
|
@taskylizard is attempting to deploy a commit to the serhalp's projects Team on Vercel. A member of the Team first needs to authorize it. |
| const parsedRoute = computed(() => { | ||
| const segments = route.params.path || [] | ||
|
|
||
| // Find the /v/ separator | ||
| const vIndex = segments.indexOf('v') | ||
| if (vIndex === -1 || vIndex >= segments.length - 1) { | ||
| return { packageName: segments.join('/'), range: null } | ||
| } | ||
|
|
||
| const packageName = segments.slice(0, vIndex).join('/') | ||
| const rangeStr = segments[vIndex + 1] ?? '' | ||
|
|
||
| // Parse version range (from...to) | ||
| const parts = rangeStr.split('...') | ||
| if (parts.length !== 2) { | ||
| return { packageName, range: null } | ||
| } | ||
|
|
||
| return { | ||
| packageName, | ||
| range: { from: parts[0]!, to: parts[1]! }, | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
cat -n app/pages/diff/[...path].vue | head -50Repository: npmx-dev/npmx.dev
Length of output: 1869
🏁 Script executed:
# Also check the project structure and Vue Router setup to understand the typing
fd -e "config\|router\|nuxt" -t f | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check if there's a tsconfig or type definitions
cat -n tsconfig.json 2>/dev/null | head -40 || echo "tsconfig.json not found"Repository: npmx-dev/npmx.dev
Length of output: 506
🏁 Script executed:
# Find nuxt.config
fd -e "nuxt.config" -t fRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check package.json to see Vue Router version
cat -n package.json | grep -A 2 -B 2 "vue-router\|nuxt" | head -30Repository: npmx-dev/npmx.dev
Length of output: 1713
🏁 Script executed:
# Look for route type definitions or RouteLocationNormalized
rg "diff.*\[\.\.\.path\]|path.*catch" -t vue -t ts --max-count 5Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Get full dependencies section from package.json
cat package.json | jq '.dependencies, .devDependencies' | grep -A 5 -B 5 "nuxt\|vue"Repository: npmx-dev/npmx.dev
Length of output: 2778
🏁 Script executed:
# Search for route definitions or definePageMeta with path param
rg "definePageMeta|useRoute" -A 3 -B 1 --max-count 10Repository: npmx-dev/npmx.dev
Length of output: 11934
🏁 Script executed:
# Check if there's a types file that defines route params
fd "types\|route" -t f -e ts -e d.ts | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check the package-code and package-docs implementations
cat -n app/pages/package-code/[...path].vue | head -40Repository: npmx-dev/npmx.dev
Length of output: 1666
🏁 Script executed:
# Check the package-docs file more carefully
cat -n app/pages/package-docs/[...path].vue | head -40Repository: npmx-dev/npmx.dev
Length of output: 1465
🏁 Script executed:
# Search for any TypeScript definitions or route type augmentation
rg "route\.params" -A 2 -B 2 --max-count 15 app/pages/Repository: npmx-dev/npmx.dev
Length of output: 1909
🏁 Script executed:
# Check nuxt.config to see the router configuration
cat -n nuxt.config.ts | head -60Repository: npmx-dev/npmx.dev
Length of output: 2148
🏁 Script executed:
# Look for unplugin-vue-router configuration or type generation
rg "unplugin-vue-router|router\.ts" -t ts -t js --max-count 10Repository: npmx-dev/npmx.dev
Length of output: 99
🏁 Script executed:
# Search for any .d.ts files or type augmentation related to routes
fd -e "d.ts" | xargs grep -l "params\|route" 2>/dev/null | head -10Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check if there's any route type definition or generated types
fd -e "routes.d.ts" -o -e "router.d.ts" -t fRepository: npmx-dev/npmx.dev
Length of output: 177
🏁 Script executed:
# Search in .nuxt generated files for route type definitions
ls -la .nuxt/types/ 2>/dev/null | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check for any imports or type annotations related to route.params in similar files
rg "route\.params\.path" -B 5 -A 5 --max-count 20Repository: npmx-dev/npmx.dev
Length of output: 2451
🏁 Script executed:
# Check Vue Router type definitions or documentation in node_modules
find node_modules -name "*router*" -type d | grep -E "vue-router|@vue" | head -5Repository: npmx-dev/npmx.dev
Length of output: 108
🏁 Script executed:
# Search for any Vue Router version info
rg "vue-router" package.jsonRepository: npmx-dev/npmx.dev
Length of output: 97
🏁 Script executed:
# Check if there's any JSDoc or TypeScript comment explaining route.params type
rg "route\.params" -B 10 -A 1 app/pages/diff/ | head -40Repository: npmx-dev/npmx.dev
Length of output: 674
🏁 Script executed:
# Check the actual TypeScript configuration and type checking setup
cat -n tsconfig.jsonRepository: npmx-dev/npmx.dev
Length of output: 506
🏁 Script executed:
# Look for any type errors or issues in tests related to route.params
rg "route\.params\|catch.*all\|\.\.\.path" test/ --max-count 20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Verify the actual behavior by checking if there are any runtime guards or type assertions
rg "Array\.isArray.*path|typeof.*path.*string" --max-count 10Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Search for any documentation or comments about how route.params.path is typed
rg "catch-all|\.\.\.path|\[\.\.\.path\]" -B 3 -A 3 --max-count 15Repository: npmx-dev/npmx.dev
Length of output: 606
🏁 Script executed:
# Check if there's any validation or type checking happening elsewhere for route params
rg "route\.params\|RouteLocationNormalized" app/ --max-count 20 -t ts -t vueRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Let's verify the actual behavior by checking if the code works as-is in the codebase
rg "segments\." app/pages/diff/\[\.\.\.path\]\.vueRepository: npmx-dev/npmx.dev
Length of output: 398
🏁 Script executed:
# Let me search for web documentation or check if there's any evidence of route.params.path being a string
# First, check if there are any error reports or TODOs related to this
rg "TODO|FIXME|BUG" app/pages/diff/\[\.\.\.path\] 2>/dev/null || echo "No specific TODOs found"Repository: npmx-dev/npmx.dev
Length of output: 84
🏁 Script executed:
# Check the actual line 24 usage to see the indexing pattern
cat -n app/pages/diff/[...path].vue | sed -n '14,36p'Repository: npmx-dev/npmx.dev
Length of output: 825
🏁 Script executed:
# Search for how route.params is documented or used in error handling
rg "route\.params\[|route\.query\[" app/ -A 1 -B 1 --max-count 10Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check if there are any type issues reported in the codebase with route.params
rg "as.*string\[\]|as.*unknown|@ts-ignore" app/pages/ | head -20Repository: npmx-dev/npmx.dev
Length of output: 313
🏁 Script executed:
# Search for any documentation or comments in the project about route param typing
cat -n app/pages/package-docs/[...path].vue | sed -n '1,30p'Repository: npmx-dev/npmx.dev
Length of output: 1054
🏁 Script executed:
# Let me check what the review comment specifically says about the issue and the coding guidelines
# Check for any actual type errors by looking at similar patterns
rg "segments\[.*\].*\?" app/pages/ -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 348
Normalise route.params.path to an array before calling array methods.
In Nuxt with catch-all routes, route.params.path should be string[], but the codebase's defensive patterns suggest potential edge cases. To ensure strict type safety per the coding guidelines, explicitly guard against the value being a string or undefined before calling .indexOf() or other array methods.
🔧 Suggested fix
- const segments = route.params.path || []
+ const rawPath = route.params.path
+ const segments = Array.isArray(rawPath) ? rawPath : rawPath ? [rawPath] : []📝 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.
| const parsedRoute = computed(() => { | |
| const segments = route.params.path || [] | |
| // Find the /v/ separator | |
| const vIndex = segments.indexOf('v') | |
| if (vIndex === -1 || vIndex >= segments.length - 1) { | |
| return { packageName: segments.join('/'), range: null } | |
| } | |
| const packageName = segments.slice(0, vIndex).join('/') | |
| const rangeStr = segments[vIndex + 1] ?? '' | |
| // Parse version range (from...to) | |
| const parts = rangeStr.split('...') | |
| if (parts.length !== 2) { | |
| return { packageName, range: null } | |
| } | |
| return { | |
| packageName, | |
| range: { from: parts[0]!, to: parts[1]! }, | |
| } | |
| }) | |
| const parsedRoute = computed(() => { | |
| const rawPath = route.params.path | |
| const segments = Array.isArray(rawPath) ? rawPath : rawPath ? [rawPath] : [] | |
| // Find the /v/ separator | |
| const vIndex = segments.indexOf('v') | |
| if (vIndex === -1 || vIndex >= segments.length - 1) { | |
| return { packageName: segments.join('/'), range: null } | |
| } | |
| const packageName = segments.slice(0, vIndex).join('/') | |
| const rangeStr = segments[vIndex + 1] ?? '' | |
| // Parse version range (from...to) | |
| const parts = rangeStr.split('...') | |
| if (parts.length !== 2) { | |
| return { packageName, range: null } | |
| } | |
| return { | |
| packageName, | |
| range: { from: parts[0]!, to: parts[1]! }, | |
| } | |
| }) |
| const selectedFromQuery = computed(() => { | ||
| const filePath = route.query.file | ||
| if (!filePath || !compare.value) return null | ||
| return allChanges.value.find(f => f.path === filePath) ?? null | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/pages/diff/[...path].vue | head -100Repository: npmx-dev/npmx.dev
Length of output: 3664
🏁 Script executed:
wc -l app/pages/diff/[...path].vueRepository: npmx-dev/npmx.dev
Length of output: 93
🏁 Script executed:
rg "route\.query\." app/pages/diff/[...path].vue -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 252
🏁 Script executed:
rg "route\.query\." --type ts --type tsx --type vue -c | head -20Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Search for patterns of handling query parameters in the codebase
rg "route\.query" app/ -A 1 | head -40Repository: npmx-dev/npmx.dev
Length of output: 2092
🏁 Script executed:
# Find the normalizeSearchParam function to understand how query params are handled elsewhere
rg "normalizeSearchParam" -A 5 --type vue | head -40Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Find the definition of normalizeSearchParam
rg "function normalizeSearchParam|const normalizeSearchParam|export.*normalizeSearchParam" --type tsRepository: npmx-dev/npmx.dev
Length of output: 178
🏁 Script executed:
cat -n shared/utils/url.ts | grep -A 10 "normalizeSearchParam"Repository: npmx-dev/npmx.dev
Length of output: 321
🏁 Script executed:
# Check Vue Router/Nuxt types to understand LocationQueryValue
rg "LocationQueryValue" --type ts -B 2 -A 2 | head -30Repository: npmx-dev/npmx.dev
Length of output: 463
Handle array-valued ?file= query parameter safely.
Vue Router's LocationQueryValue can be string | string[] | null, but the current code accesses route.query.file directly without guarding for arrays. If a user passes multiple file parameters, this will fail the comparison check. Extract the first element from arrays before use.
🔧 Suggested fix
- const filePath = route.query.file
- if (!filePath || !compare.value) return null
- return allChanges.value.find(f => f.path === filePath) ?? null
+ const fileParam = route.query.file
+ const filePath = Array.isArray(fileParam) && fileParam.length > 0 ? fileParam[0] : fileParam
+ if (!filePath || !compare.value) return null
+ return allChanges.value.find(f => f.path === filePath) ?? nullAligns with coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
📝 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.
| const selectedFromQuery = computed(() => { | |
| const filePath = route.query.file | |
| if (!filePath || !compare.value) return null | |
| return allChanges.value.find(f => f.path === filePath) ?? null | |
| }) | |
| const selectedFromQuery = computed(() => { | |
| const fileParam = route.query.file | |
| const filePath = Array.isArray(fileParam) && fileParam.length > 0 ? fileParam[0] : fileParam | |
| if (!filePath || !compare.value) return null | |
| return allChanges.value.find(f => f.path === filePath) ?? null | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/diff/FileTree.vue (1)
47-50: Avoid non‑null assertions when indexingparts.Use
entries()to keep index access type‑safe and remove the non‑null assertion.As per coding guidelines: Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index.♻️ Suggested refactor
- for (let i = 0; i < parts.length; i++) { - const part = parts[i]! + for (const [i, part] of parts.entries()) { const isFile = i === parts.length - 1 const path = parts.slice(0, i + 1).join('/')
| // Sort: directories first, then alphabetically | ||
| function sortTree(nodes: DiffTreeNode[]): DiffTreeNode[] { | ||
| return nodes | ||
| .map(n => ({ | ||
| ...n, | ||
| children: n.children ? sortTree(n.children) : undefined, | ||
| })) | ||
| .sort((a, b) => { | ||
| if (a.type !== b.type) return a.type === 'directory' ? -1 : 1 | ||
| return a.name.localeCompare(b.name) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file
fd "FileTree.vue" --type fRepository: npmx-dev/npmx.dev
Length of output: 126
🏁 Script executed:
# Read the file to examine lines 26-36 and surrounding context
cat -n app/components/diff/FileTree.vue | head -80Repository: npmx-dev/npmx.dev
Length of output: 2706
🏁 Script executed:
# Check if there's SSR/Nuxt context and how other sorting is done
rg -A 3 -B 3 "Intl\.Collator|localeCompare" --type ts --type vueRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check what framework this is (Nuxt, Next, etc.)
head -20 package.jsonRepository: npmx-dev/npmx.dev
Length of output: 664
🏁 Script executed:
# Search for localeCompare and Intl.Collator patterns correctly
rg "localeCompare|Intl\.Collator" --type-list | grep -i vueRepository: npmx-dev/npmx.dev
Length of output: 450
🏁 Script executed:
# Better approach - search without specific type
rg "localeCompare|Intl\.Collator" -lRepository: npmx-dev/npmx.dev
Length of output: 693
🏁 Script executed:
# Check if this is an SSR-heavy Nuxt app (check nuxt.config)
cat nuxt.config.ts 2>/dev/null || cat nuxt.config.js 2>/dev/null || echo "Config not found in standard locations"Repository: npmx-dev/npmx.dev
Length of output: 8056
🏁 Script executed:
# Search for other localeCompare usage
rg "localeCompare" -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 12193
🏁 Script executed:
# Check if there are similar type-safety issues with array access assertions
rg "parts\[|children\!" -B 1 -A 1 app/components/diff/FileTree.vueRepository: npmx-dev/npmx.dev
Length of output: 161
🏁 Script executed:
# Look at server-side implementation for reference
cat -n server/utils/file-tree.ts | head -100Repository: npmx-dev/npmx.dev
Length of output: 2932
🏁 Script executed:
# Verify the Intl.Collator approach is standard in the codebase (check if already used)
rg "Intl\.Collator" app/ server/Repository: npmx-dev/npmx.dev
Length of output: 43
Make tree sorting deterministic across SSR and client locales.
localeCompare without an explicit locale can sort differently between Node.js (server-side rendering) and the browser, which risks hydration mismatches and inconsistent ordering. Use a fixed Intl.Collator to ensure the tree order is stable across both environments.
💡 Suggested fix
+const treeNameCollator = new Intl.Collator('en', { numeric: true, sensitivity: 'base' })
+
function sortTree(nodes: DiffTreeNode[]): DiffTreeNode[] {
return nodes
.map(n => ({
...n,
children: n.children ? sortTree(n.children) : undefined,
}))
.sort((a, b) => {
if (a.type !== b.type) return a.type === 'directory' ? -1 : 1
- return a.name.localeCompare(b.name)
+ return treeNameCollator.compare(a.name, b.name)
})
}| // Build tree structure from flat file list (only at root level) | ||
| function buildTree(files: FileChange[]): DiffTreeNode[] { | ||
| const root: DiffTreeNode[] = [] | ||
| for (const file of files) { | ||
| const parts = file.path.split('/') | ||
| let current = root | ||
| for (let i = 0; i < parts.length; i++) { | ||
| const part = parts[i]! | ||
| const isFile = i === parts.length - 1 | ||
| const path = parts.slice(0, i + 1).join('/') | ||
| let node = current.find(n => n.name === part) | ||
| if (!node) { | ||
| node = { | ||
| name: part, | ||
| path, | ||
| type: isFile ? 'file' : 'directory', | ||
| changeType: isFile ? file.type : undefined, | ||
| children: isFile ? undefined : [], | ||
| } | ||
| current.push(node) | ||
| } | ||
| if (!isFile) { | ||
| current = node.children! | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd app/components/diff && cat -n FileTree.vue | head -100Repository: npmx-dev/npmx.dev
Length of output: 3354
🏁 Script executed:
fd -t f "types" --extension ts --extension tsx | head -20Repository: npmx-dev/npmx.dev
Length of output: 104
🏁 Script executed:
rg "type FileChange" --type ts --type tsx -A 5Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
rg "type FileChange" -A 5Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
find . -name "types*" -type f | grep -i sharedRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
rg "FileChange" --type ts -B 2 -A 3 | head -50Repository: npmx-dev/npmx.dev
Length of output: 1836
🏁 Script executed:
cat -n shared/types/compare.ts | head -40Repository: npmx-dev/npmx.dev
Length of output: 1576
🏁 Script executed:
cat -n server/utils/compare.ts | head -120Repository: npmx-dev/npmx.dev
Length of output: 4247
🏁 Script executed:
rg "buildTree" -B 3 -A 3Repository: npmx-dev/npmx.dev
Length of output: 955
🏁 Script executed:
# Let me trace through a potential collision scenario
# Check if test files exist for FileTree
find . -name "*FileTree*" -type fRepository: npmx-dev/npmx.dev
Length of output: 168
🏁 Script executed:
# Let me examine the data flow more carefully
# Check where buildTree is called and what data is passed to it
grep -r "buildTree" app/components/diff/FileTree.vue -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 406
🏁 Script executed:
# Let me create a test scenario to understand the data flow
# Trace through compareFileTrees to see if it can produce conflicting paths
cat -n server/utils/compare.ts | sed -n '49,95p'Repository: npmx-dev/npmx.dev
Length of output: 1468
🏁 Script executed:
# Let me verify the collision scenario with a concrete example
# If the files array contains both 'foo' and 'foo/bar', the buildTree function will fail
python3 << 'EOF'
# Simulate the buildTree function with a collision scenario
files = [
{'path': 'foo', 'type': 'added'},
{'path': 'foo/bar', 'type': 'added'}
]
root = []
for file in files:
parts = file['path'].split('/')
current = root
for i, part in enumerate(parts):
is_file = i == len(parts) - 1
path = '/'.join(parts[:i+1])
# Find existing node
node = None
for n in current:
if n['name'] == part:
node = n
break
if not node:
node = {
'name': part,
'path': path,
'type': 'file' if is_file else 'directory',
'children': None if is_file else []
}
current.append(node)
if not is_file:
# This is where the error would occur in TypeScript
if node['children'] is None:
print(f"ERROR: Tried to access .children on file node '{node['name']}' at path '{node['path']}'")
print(f"This happens when processing file path '{file['path']}'")
print(f"Node structure: {node}")
break
current = node['children']
print(f"Processed: {file['path']} -> part='{part}', isFile={is_file}, nodeType={node['type']}")
print("\nConclusion: Collision DOES occur with paths 'foo' and 'foo/bar'")
EOFRepository: npmx-dev/npmx.dev
Length of output: 378
🏁 Script executed:
# Check if such conflicting paths can realistically occur in npm packages
# by examining real package structures or test data
find . -path "*/test*" -name "*.ts" -type f | xargs grep -l "compareFileTrees\|FileChange" | head -5Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check the PackageFileTree type to understand the data structure better
rg "interface PackageFileTree|type PackageFileTree" -A 10Repository: npmx-dev/npmx.dev
Length of output: 1211
🏁 Script executed:
# Now let me verify if this scenario can actually occur in real npm packages
# by checking how compareFileTrees would generate such conflicting FileChange entries
cat -n server/utils/compare.ts | sed -n '58,85p'Repository: npmx-dev/npmx.dev
Length of output: 946
Handle potential file–directory name collisions in the tree builder.
The buildTree function can fail if the input contains both a file and a directory with the same name segment (e.g., foo as a file and foo/bar as a file). This occurs because when foo is created as a file node with children: undefined, the subsequent attempt to access node.children! on line 65 will throw an error.
This scenario is possible in real package comparisons when a directory is replaced by a file or vice versa. Add a type guard to verify that the node is a directory before accessing its children property, or skip/merge conflicting paths explicitly.
The diff viewer was not showing green background highlighting for added
lines in modified files that only had additions (no deletions), like
`src/constants.js` in svelte 5.16.2→5.50.0.
Root cause:
1. `parseUnifiedDiff` used a heuristic that reclassified files based on
diff content: if a file had only insertions, it was marked as 'add'
(new file). This was wrong for modifications with only additions.
2. `computeDiff` in ViewerPanel used `parsed.type` ('add') instead of
the correctly computed `type` ('modify') based on file existence.
3. DiffLine intentionally skips green background when fileStatus='add'
(all lines in a new file are additions, so highlighting is redundant).
This caused addition-only modifications to lose visual highlighting.
Fix:
- `parseUnifiedDiff`: Replace the content-based heuristic with proper
/dev/null path detection from --- / +++ headers, which is the standard
way unified diffs indicate new/deleted files.
- `ViewerPanel.vue`: Use the file-existence-based type instead of the
parsed diff type.
|
Checklist for this PR:
|
- SidebarPanel.vue: Replace bare `/${dep.name}` dependency links with
`packageRoute(dep.name)` which resolves to /package/:org?/:name
- ViewerPanel.vue: Update code browser URLs from /code/ alias to
canonical /package-code/ path
- diff/[...path].vue: Remove duplicated local packageRoute() function
and use the shared utility from ~/utils/router instead
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/pages/diff/[...path].vue (2)
28-36: Guard against empty array elements after split.The
partsarray could contain empty strings if the version range is malformed (e.g.,...5.0.0or4.0.0...). The non-null assertions on lines 35 are safe from an array-bounds perspective due to the length check, but the values could be empty strings, which may cause downstream issues.🛡️ Proposed defensive check
const parts = rangeStr.split('...') - if (parts.length !== 2) { + if (parts.length !== 2 || !parts[0] || !parts[1]) { return { packageName, range: null } } return { packageName, range: { from: parts[0]!, to: parts[1]! }, }
201-208: Redundant event emission alongside v-model binding.The
DiffSidebarPanelalready hasv-model:selected-file="selectedFile"for two-way binding. The additional@file-select="selectedFile = $event"handler is redundant and may cause double-updates.♻️ Proposed fix
<DiffSidebarPanel :compare="compare" :grouped-deps="groupedDeps" :all-changes="allChanges" v-model:selected-file="selectedFile" v-model:file-filter="fileFilter" - `@file-select`="selectedFile = $event" />app/components/diff/SidebarPanel.vue (1)
21-22: Consider using auto-imported$t()instead of destructuring fromuseI18n().Based on learnings, this Nuxt 4 project with nuxtjs/i18n v10 exposes
$t()globally in<script setup>. The pattern across other pages is to rely on the auto-imported$t()rather than destructuringtfromuseI18n(). However, both approaches work correctly.♻️ Proposed change for consistency
const sectionOrder = ['dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies'] -const { t } = useI18n() const sectionMeta = computed<Record<string, { label: string; icon: string }>>(() => ({ - dependencies: { label: t('compare.dependencies'), icon: 'i-carbon-cube' }, - devDependencies: { label: t('compare.dev_dependencies'), icon: 'i-carbon-tools' }, - peerDependencies: { label: t('compare.peer_dependencies'), icon: 'i-carbon-user-multiple' }, - optionalDependencies: { label: t('compare.optional_dependencies'), icon: 'i-carbon-help' }, + dependencies: { label: $t('compare.dependencies'), icon: 'i-carbon-cube' }, + devDependencies: { label: $t('compare.dev_dependencies'), icon: 'i-carbon-tools' }, + peerDependencies: { label: $t('compare.peer_dependencies'), icon: 'i-carbon-user-multiple' }, + optionalDependencies: { label: $t('compare.optional_dependencies'), icon: 'i-carbon-help' }, }))Based on learnings: "In this Nuxt 4 project with nuxtjs/i18n v10, $t() and other globals like $n, $d are exposed in <script setup>... Do not destructure t from useI18n(); rely on the global provided by Nuxt i18n in script setup."
| <div class="relative"> | ||
| <span | ||
| class="absolute left-2 top-1/2 -translate-y-1/2 i-carbon-search w-3 h-3 text-fg-subtle pointer-events-none" | ||
| /> | ||
| <input | ||
| v-model="fileSearch" | ||
| type="search" | ||
| :placeholder="$t('compare.search_files_placeholder')" | ||
| :aria-label="$t('compare.search_files_placeholder')" | ||
| class="w-full text-[11px] pl-6.5 pr-2 py-1 bg-bg-subtle border border-border rounded font-mono placeholder:text-fg-subtle transition-colors hover:border-border-hover focus:border-accent focus:outline-none" |
There was a problem hiding this comment.
Fix RTL compliance: replace physical positioning/padding with logical properties.
The pipeline reports multiple RTL linting errors on these lines. Use logical properties for proper right-to-left language support:
left-2→inset-is-2(inset-inline-start)pl-6.5→ps-6.5(padding-inline-start)pr-2→pe-2(padding-inline-end)
🔧 Proposed fix
<div class="relative">
<span
- class="absolute left-2 top-1/2 -translate-y-1/2 i-carbon-search w-3 h-3 text-fg-subtle pointer-events-none"
+ class="absolute inset-is-2 top-1/2 -translate-y-1/2 i-carbon-search w-3 h-3 text-fg-subtle pointer-events-none"
/>
<input
v-model="fileSearch"
type="search"
:placeholder="$t('compare.search_files_placeholder')"
:aria-label="$t('compare.search_files_placeholder')"
- class="w-full text-[11px] pl-6.5 pr-2 py-1 bg-bg-subtle border border-border rounded font-mono placeholder:text-fg-subtle transition-colors hover:border-border-hover focus:border-accent focus:outline-none"
+ class="w-full text-[11px] ps-6.5 pe-2 py-1 bg-bg-subtle border border-border rounded font-mono placeholder:text-fg-subtle transition-colors hover:border-border-hover focus:border-accent focus:outline-none"
/>
</div>🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 201-201: RTL: avoid using 'left-2', use 'inset-is-2' instead.
[error] 208-208: RTL: avoid using 'pl-6.5', use 'ps-6.5' instead.
[error] 208-208: RTL: avoid using 'pr-2', use 'pe-2' instead.
4 participants
Closes #49