fix: wrap code viewer lines by computing width#2070
fix: wrap code viewer lines by computing width#2070nosthrillz wants to merge 7 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
| 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; |
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 :)