fix: wrap code viewer lines by computing width#2070
fix: wrap code viewer lines by computing width#2070nosthrillz wants to merge 8 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks the Code Viewer to support wrapped lines by tracking per-line heights with a reactive Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db7d3812-0584-449a-ac8f-5830e35c5103
📒 Files selected for processing (2)
app/components/Code/Viewer.vueapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
Show resolved
Hide resolved
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)
588-603:@screenusage now correct; minor responsiveness consideration remains.The
@screen lgdirective is correctly placed at top-level (not nested), addressing the previous review feedback. The switch from100vwto100%is also appropriate.However,
.file-viewerwidth applies at all breakpoints, whilst.file-treeis only visible onmd+screens. On mobile, this subtracts sidebar space for a hidden element. Theflex-1class should compensate viaflex-grow, but consider scoping the width rule tomd+for clarity:Option: Scope width rules to md+ breakpoint
.file-tree { width: var(--sidebar-space); } +@screen md { + .file-viewer { + width: calc(100% - var(--sidebar-space)); + } +} -.file-viewer { - width: calc(100% - var(--sidebar-space)); -}Alternatively, since
.file-vieweralready hasflex-1 min-w-0in the template, the explicit width may be unnecessary if flexbox sizing is sufficient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d420e8f0-1328-42ad-968d-7926a66f68b3
📒 Files selected for processing (2)
app/components/Code/Viewer.vueapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
|
A toggle would be nice and also line numbers should adapt Example comparison https://npmx.dev/package-code/grammy/v/1.41.1/out%2Fbot.d.ts https://npmx-axzqrskfc-npmx.vercel.app/package-code/grammy/v/1.41.1/out%2Fbot.d.ts |
|
Switching to Draft because the line numbers need to be recomputed. As attempted in |
|
the wrapping is very readable and atm I also only see the issue of the line numbers & the highlighting included with it. a file that may be good to check would be in nuxt the /dist/index.mjs file because it has large imports |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Code/Viewer.vue (1)
30-30: Consider debouncing the resize observer callback.
updateLineMultipliersqueriesgetComputedStylefor every line element. For large files (as mentioned in PR comments regarding Nuxt's/dist/index.mjs), this could cause performance issues during continuous resize events.♻️ Suggested debounce implementation
+import { useDebounceFn } from '@vueuse/core' + +const debouncedUpdateLineMultipliers = useDebounceFn(updateLineMultipliers, 100) + -useResizeObserver(codeRef, updateLineMultipliers) +useResizeObserver(codeRef, debouncedUpdateLineMultipliers)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59176b83-d3f1-482d-a169-c55c5dd5d568
📒 Files selected for processing (1)
app/components/Code/Viewer.vue
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e43e32c-4ae1-4c98-9eb3-4e11d0ded3ee
📒 Files selected for processing (2)
app/utils/chart-data-buckets.tstest/unit/app/utils/chart-data-buckets.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/Code/Viewer.vue (1)
26-30: Consider consolidating duplicateprops.htmlwatchers.Both watchers react to the same source and schedule
nextTickwork. Merging them into a single watcher would simplify ordering and reduce reactive overhead.♻️ Proposed refactor
-watch( - () => props.html, - () => nextTick(updateLineMultipliers), - { immediate: true }, -) useResizeObserver(codeRef, updateLineMultipliers) @@ -watch( - () => props.html, - () => { - nextTick(handleImportLinkNavigate) - }, - { immediate: true }, -) +watch( + () => props.html, + () => { + nextTick(() => { + updateLineMultipliers() + handleImportLinkNavigate() + }) + }, + { immediate: true }, +)Also applies to: 101-107
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cad5e82e-58c5-4e74-b233-5ed5eeac11fd
📒 Files selected for processing (1)
app/components/Code/Viewer.vue
app/components/Code/Viewer.vue
Outdated
| function updateLineMultipliers() { | ||
| if (!codeRef.value) return | ||
| const lines = Array.from(codeRef.value.querySelectorAll('code > .line')) | ||
| lineMultipliers.value = lines.map(line => | ||
| Math.max(1, Math.round(parseFloat(getComputedStyle(line).height) / LINE_HEIGHT_PX)), | ||
| ) |
There was a problem hiding this comment.
Throttle resize-driven line measurement to avoid jank on large files.
Line 31 currently triggers a full recomputation, and Lines 20-23 read computed style for every rendered code line each time. On large files this can become expensive during resize and cause visible lag.
⚡ Proposed fix
+const scheduleLineMultiplierUpdate = useDebounceFn(() => {
+ updateLineMultipliers()
+}, 16)
+
watch(
() => props.html,
- () => nextTick(updateLineMultipliers),
+ () => nextTick(scheduleLineMultiplierUpdate),
{ immediate: true },
)
-useResizeObserver(codeRef, updateLineMultipliers)
+useResizeObserver(codeRef, scheduleLineMultiplierUpdate)Also applies to: 31-31
| .code-content:deep(.line) { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| /* Ensure consistent height matching line numbers */ | ||
| line-height: 24px; | ||
| min-height: 24px; | ||
| max-height: 24px; | ||
| white-space: pre; | ||
| line-height: calc(v-bind(LINE_HEIGHT_PX) * 1px); | ||
| min-height: calc(v-bind(LINE_HEIGHT_PX) * 1px); | ||
| white-space: pre-wrap; | ||
| overflow: hidden; |
There was a problem hiding this comment.
Long unbroken tokens can still overflow in wrap mode.
Line 185 uses white-space: pre-wrap, which does not hard-break long strings without natural breakpoints (e.g. minified blobs, hashes, base64). This can reintroduce horizontal overflow despite the wrapping objective.
🩹 Proposed fix
.code-content:deep(.line) {
display: flex;
flex-wrap: wrap;
/* Ensure consistent height matching line numbers */
line-height: calc(v-bind(LINE_HEIGHT_PX) * 1px);
min-height: calc(v-bind(LINE_HEIGHT_PX) * 1px);
white-space: pre-wrap;
+ overflow-wrap: anywhere;
+ word-break: break-word;
overflow: hidden;
transition: background-color 0.1s;
max-width: 100%;
}📝 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.
| .code-content:deep(.line) { | |
| display: flex; | |
| flex-wrap: wrap; | |
| /* Ensure consistent height matching line numbers */ | |
| line-height: 24px; | |
| min-height: 24px; | |
| max-height: 24px; | |
| white-space: pre; | |
| line-height: calc(v-bind(LINE_HEIGHT_PX) * 1px); | |
| min-height: calc(v-bind(LINE_HEIGHT_PX) * 1px); | |
| white-space: pre-wrap; | |
| overflow: hidden; | |
| .code-content:deep(.line) { | |
| display: flex; | |
| flex-wrap: wrap; | |
| /* Ensure consistent height matching line numbers */ | |
| line-height: calc(v-bind(LINE_HEIGHT_PX) * 1px); | |
| min-height: calc(v-bind(LINE_HEIGHT_PX) * 1px); | |
| white-space: pre-wrap; | |
| overflow-wrap: anywhere; | |
| word-break: break-word; | |
| overflow: hidden; |
|
After spending hours on this, I've hit a roadblock. This is a non-trivial issue, even though it seemed like "bite-sized" initially. Been getting massive performance hits on large files due to recomputing line heights for 10^3 .. 10^5 nodes if you count in the highlighting. What I tried:
The jist of it is that, to wrap the 8500 Shiki-highlighted lines, the browser needs a lot of time counting line heights, and reconciling, because we need the window resize, so it's not a one-off Going forward:
IMHO I think the GitHub style option fits the UX the closest. Please advise |
| const rangeEndDate = parseIsoDate(rangeEndIso) | ||
|
|
||
| // Align from last day with actual data (npm has 1-2 day delay, today is incomplete) | ||
| const lastNonZero = sorted.findLast(d => d.value > 0) | ||
| const pickerEnd = parseIsoDate(rangeEndIso) | ||
| const effectiveEnd = lastNonZero ? parseIsoDate(lastNonZero.day) : pickerEnd | ||
| const rangeEndDate = effectiveEnd.getTime() < pickerEnd.getTime() ? effectiveEnd : pickerEnd | ||
|
|
||
| // Group into 7-day buckets from END backwards | ||
| const buckets = new Map<number, number>() | ||
|
|
||
| for (const item of sorted) { | ||
| const offset = Math.floor((rangeEndDate.getTime() - parseIsoDate(item.day).getTime()) / DAY_MS) | ||
| const itemDate = parseIsoDate(item.day) | ||
| const offset = Math.floor((rangeEndDate.getTime() - itemDate.getTime()) / DAY_MS) |
There was a problem hiding this comment.
Are these changes intended in this PR?
There was a problem hiding this comment.
They are not, a mistake on my part for merging latest instead of rebasing :)
I will have to remove them. Don't think this PR will end up being merged to be honest, given the above conversation
4 participants
🔗 Linked issue
Attempts to resolve 2027
🧭 Context
The current approach has overflow-x-auto, but wrapping might be better.
It's unclear whether that's preferred, so making the PR to allow a preview.
📚 Description
We actually can compute available screen width, so we can avoid overflow, and instead set the width and use flex-wrap combined with pre-wrap to get the desired effect.
Potential points of contention / concerns
I didn't visually test the highlighting, I'm yet unsure what the implications would be :)