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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
app/components/diff/ViewerPanel.vue (2)
50-50:⚠️ Potential issue | 🟠 MajorEncode
file.pathbefore composing external/internal URLs.Raw paths with spaces or reserved characters can produce false 404s and broken “View file” links.
🔧 Proposed fix
async function fetchFileContent(version: string): Promise<string | null> { const controller = new AbortController() const timeoutId = setTimeout(() => controller.abort(), DIFF_TIMEOUT) - const url = `https://cdn.jsdelivr.net/npm/${props.packageName}@${version}/${props.file.path}` + const safePath = props.file.path.replace(/^\/+/, '') + const encodedPath = safePath.split('/').map(encodeURIComponent).join('/') + const url = `https://cdn.jsdelivr.net/npm/${props.packageName}@${version}/${encodedPath}` @@ function getCodeUrl(version: string): string { - return `/package-code/${props.packageName}/v/${version}/${props.file.path}` + const safePath = props.file.path.replace(/^\/+/, '') + const encodedPath = safePath.split('/').map(encodeURIComponent).join('/') + return `/package-code/${props.packageName}/v/${version}/${encodedPath}` }Also applies to: 215-217
158-161:⚠️ Potential issue | 🟡 MinorNormalise thrown values before assigning
loadError.Casting with
as Erroris unsafe when non-Errorvalues are thrown.🔧 Proposed fix
} catch (err) { if (token !== loadToken.value) return - loadError.value = err as Error + loadError.value = err instanceof Error ? err : new Error(String(err)) diff.value = null } finally {Based on learnings: Applies to **/*.{ts,tsx,vue} : Use error handling patterns consistently.
lunaria/files/en-US.json (1)
1065-1077:⚠️ Potential issue | 🟡 MinorAdd singular/plural variants for count strings.
These keys still render ungrammatical singular output for count = 1.
🔧 Proposed fix
- "files_count": "{count} files", + "files_count": "{count} file | {count} files", @@ - "lines_hidden": "{count} lines hidden", + "lines_hidden": "{count} line hidden | {count} lines hidden", @@ - "deps_count": "{count} deps", + "deps_count": "{count} dep | {count} deps",lunaria/files/en-GB.json (1)
1065-1077:⚠️ Potential issue | 🟡 MinorPluralise the count labels in en-GB as well.
files_count,lines_hidden, anddeps_countneed singular/plural forms to avoid “1 files/deps/lines”.🔧 Proposed fix
- "files_count": "{count} files", + "files_count": "{count} file | {count} files", @@ - "lines_hidden": "{count} lines hidden", + "lines_hidden": "{count} line hidden | {count} lines hidden", @@ - "deps_count": "{count} deps", + "deps_count": "{count} dep | {count} deps",i18n/locales/en.json (1)
1066-1077:⚠️ Potential issue | 🟡 MinorReinstate plural forms for count-based diff labels.
This block regresses to single-form count strings and will render incorrect singular phrasing.
🔧 Proposed fix
- "files_count": "{count} files", + "files_count": "{count} file | {count} files", @@ - "lines_hidden": "{count} lines hidden", + "lines_hidden": "{count} line hidden | {count} lines hidden", @@ - "deps_count": "{count} deps", + "deps_count": "{count} dep | {count} deps",
🧹 Nitpick comments (1)
app/components/diff/ViewerPanel.vue (1)
584-584: Use logical property for RTL-safe handle positioning.Using physical
rightcan misplace the slider accent in RTL layouts.🔧 Proposed fix
.slider-range::after { content: ''; position: absolute; - right: 8px; + inset-inline-end: 8px; top: 50%; width: 2px;
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/components/diff/ViewerPanel.vueapp/pages/package/[[org]]/[name].vuei18n/locales/en.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.json
| </template> | ||
|
|
||
| <!-- File sizes --> | ||
| <span v-if="file.oldSize || file.newSize" class="text-xs text-fg-subtle shrink-0"> |
There was a problem hiding this comment.
Show 0-byte file sizes correctly.
file.oldSize || file.newSize hides valid 0 values, so empty files lose size metadata in the header.
🔧 Proposed fix
-<span v-if="file.oldSize || file.newSize" class="text-xs text-fg-subtle shrink-0">
+<span v-if="file.oldSize != null || file.newSize != null" class="text-xs text-fg-subtle 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.
| <span v-if="file.oldSize || file.newSize" class="text-xs text-fg-subtle shrink-0"> | |
| <span v-if="file.oldSize != null || file.newSize != null" class="text-xs text-fg-subtle shrink-0"> |
| <button | ||
| type="button" | ||
| class="px-2 py-1 text-xs text-fg-muted hover:text-fg bg-bg-muted border border-border rounded transition-colors flex items-center gap-1.5" | ||
| :class="{ 'bg-bg-elevated text-fg': showOptions }" | ||
| @click="showOptions = !showOptions" | ||
| > | ||
| <span class="i-carbon-settings w-3.5 h-3.5" /> | ||
| Options | ||
| <span | ||
| class="i-carbon-chevron-down w-3 h-3 transition-transform" | ||
| :class="{ 'rotate-180': showOptions }" | ||
| /> | ||
| </button> |
There was a problem hiding this comment.
Add ARIA state to the options toggle button.
The dropdown trigger is missing explicit expanded/controls semantics, which hurts screen-reader navigation.
🔧 Proposed fix
<button
type="button"
+ aria-label="Diff options"
+ :aria-expanded="showOptions"
+ aria-controls="diff-options-panel"
class="px-2 py-1 text-xs text-fg-muted hover:text-fg bg-bg-muted border border-border rounded transition-colors flex items-center gap-1.5"
:class="{ 'bg-bg-elevated text-fg': showOptions }"
`@click`="showOptions = !showOptions"
>
@@
<motion.div
v-if="showOptions"
+ id="diff-options-panel"
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.
| <button | |
| type="button" | |
| class="px-2 py-1 text-xs text-fg-muted hover:text-fg bg-bg-muted border border-border rounded transition-colors flex items-center gap-1.5" | |
| :class="{ 'bg-bg-elevated text-fg': showOptions }" | |
| @click="showOptions = !showOptions" | |
| > | |
| <span class="i-carbon-settings w-3.5 h-3.5" /> | |
| Options | |
| <span | |
| class="i-carbon-chevron-down w-3 h-3 transition-transform" | |
| :class="{ 'rotate-180': showOptions }" | |
| /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label="Diff options" | |
| :aria-expanded="showOptions" | |
| aria-controls="diff-options-panel" | |
| class="px-2 py-1 text-xs text-fg-muted hover:text-fg bg-bg-muted border border-border rounded transition-colors flex items-center gap-1.5" | |
| :class="{ 'bg-bg-elevated text-fg': showOptions }" | |
| `@click`="showOptions = !showOptions" | |
| > | |
| <span class="i-carbon-settings w-3.5 h-3.5" /> | |
| Options | |
| <span | |
| class="i-carbon-chevron-down w-3 h-3 transition-transform" | |
| :class="{ 'rotate-180': showOptions }" | |
| /> | |
| </button> | |
| <motion.div | |
| v-if="showOptions" | |
| id="diff-options-panel" | |
| class="absolute inset-ie-0 top-full mt-2 z-20 p-4 bg-bg-elevated border border-border shadow-2xl overflow-auto" |
| <LinkBase | ||
| v-if=" | ||
| displayVersion && latestVersion && displayVersion.version !== latestVersion.version | ||
| " | ||
| variant="button-secondary" | ||
| :to="`/diff/${pkg.name}/v/${displayVersion.version}...${latestVersion.version}`" | ||
| classicon="i-carbon-text-align-mixed" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find IconClass type definition
rg -nP "type\s+IconClass|export\s+type\s+IconClass" -C 3
# Search for icon usage patterns in the specific file
rg -n "classicon=" app/pages/package/[[org]]/[name].vue | head -20
# Check how icons with colon syntax are used in the project
rg -n "i-carbon:" --type vue | head -30
# Check how icons with dash syntax are used
rg -n "i-carbon-" --type vue | head -30Repository: npmx-dev/npmx.dev
Length of output: 1505
Use a valid IconClass token for the new diff action.
The classicon="i-carbon-text-align-mixed" attribute fails type-checking because IconClass type permits only colon syntax (e.g., i-carbon:${string}). All other icon usages in this file and throughout the project use colon syntax.
Proposed fix
- classicon="i-carbon-text-align-mixed"
+ classicon="i-carbon:text-align-mixed"📝 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.
| <LinkBase | |
| v-if=" | |
| displayVersion && latestVersion && displayVersion.version !== latestVersion.version | |
| " | |
| variant="button-secondary" | |
| :to="`/diff/${pkg.name}/v/${displayVersion.version}...${latestVersion.version}`" | |
| classicon="i-carbon-text-align-mixed" | |
| > | |
| <LinkBase | |
| v-if=" | |
| displayVersion && latestVersion && displayVersion.version !== latestVersion.version | |
| " | |
| variant="button-secondary" | |
| :to="`/diff/${pkg.name}/v/${displayVersion.version}...${latestVersion.version}`" | |
| classicon="i-carbon:text-align-mixed" | |
| > |
🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 887-887:
Type '"i-carbon-text-align-mixed"' is not assignable to type 'IconClass | undefined'.
app/pages/package/[[org]]/[name].vue
Outdated
| <li v-if="resolvedVersion" class="sm:hidden"> | ||
| <LinkBase | ||
| :to="{ name: 'code', params: { path: [pkg.name, 'v', resolvedVersion] } }" | ||
| classicon="i-carbon:code" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the file and get basic info
fd -t f "name\.vue" app/pages/package/
# Read the specific file to understand the context
wc -l app/pages/package/[[org]]/[name].vueRepository: npmx-dev/npmx.dev
Length of output: 102
🏁 Script executed:
#!/bin/bash
# Read the area around lines 1029-1033 and find codeLink definition
sed -n '1020,1045p' 'app/pages/package/[[org]]/[name].vue'Repository: npmx-dev/npmx.dev
Length of output: 1068
🏁 Script executed:
#!/bin/bash
# Search for codeLink computed property definition in the file
rg -n "codeLink" 'app/pages/package/[[org]]/[name].vue' -B2 -A5Repository: npmx-dev/npmx.dev
Length of output: 958
🏁 Script executed:
#!/bin/bash
# Search for code route definition and how it's used elsewhere
rg -n "name:\s*['\"]code['\"]" --type=vue -B3 -A3Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Check router configuration to understand the code route structure
fd -t f "router\|route" --extension=ts --extension=js | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Get the full codeLink computed definition
sed -n '678,695p' 'app/pages/package/[[org]]/[name].vue'Repository: npmx-dev/npmx.dev
Length of output: 557
🏁 Script executed:
#!/bin/bash
# Search for where code route is defined/configured
rg -n "code.*route\|route.*code" --type=ts --type=js | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for the router configuration file
fd -type f "router\|routes" | grep -E "\.(ts|js|vue)$" | head -10Repository: npmx-dev/npmx.dev
Length of output: 231
Fix mobile "Code" link to use typed route parameters.
The mobile link uses an incorrect parameter shape (params.path as array) that doesn't match the code route definition, which expects org, packageName, version, and filePath. Additionally, it doesn't properly handle scoped package names. Use the existing codeLink computed object instead, which already handles these concerns and provides proper type safety.
Proposed fix
- <li v-if="resolvedVersion" class="sm:hidden">
+ <li v-if="codeLink" class="sm:hidden">
<LinkBase
- :to="{ name: 'code', params: { path: [pkg.name, 'v', resolvedVersion] } }"
+ :to="codeLink"
classicon="i-carbon:code"
>
{{ $t('package.links.code') }}
</LinkBase>
</li>📝 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.
| <li v-if="resolvedVersion" class="sm:hidden"> | |
| <LinkBase | |
| :to="{ name: 'code', params: { path: [pkg.name, 'v', resolvedVersion] } }" | |
| classicon="i-carbon:code" | |
| > | |
| <li v-if="codeLink" class="sm:hidden"> | |
| <LinkBase | |
| :to="codeLink" | |
| classicon="i-carbon:code" | |
| > | |
| {{ $t('package.links.code') }} | |
| </LinkBase> | |
| </li> |
🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 1031-1031:
Type '{ name: "code"; params: { path: string[]; }; }' is not assignable to type 'RouteLocationRaw | undefined'.
# Conflicts: # test/nuxt/a11y.spec.ts
4 participants
Closes #49