Conversation
- Map text/plain to new 'text' type instead of 'docs' to skip LibreOffice conversion - Add 'text' to SUPPORTED_DOCUMENT_SIMPLE_TYPES Co-authored-by: marcftone <marcftone@gmail.com>
- Render text pages on HTML5 Canvas for consistent, non-selectable display - Deterministic pagination: 80 chars/line, 52 lines/page, word-wrapped - Keyboard nav (arrow keys), zoom controls, screenshot protection - Page view tracking via useSafePageViewTracker - Route fileType 'text' to TextViewer in view-data.tsx Co-authored-by: marcftone <marcftone@gmail.com>
- Return file URL for 'text' type in views API (file resolution + response) - Add text type handling in preview-data API endpoint - Add PreviewTextViewer component for dashboard preview - Route text files in preview-viewer.tsx - Add FileTextIcon for 'text' type in get-file-icon.tsx Co-authored-by: marcftone <marcftone@gmail.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR adds support for plain text file type throughout the application. Changes include creating text pagination utilities, implementing text viewer components for both preview and full view modes, extending API routes to handle text document versions, updating type mappings and constants, and adding text file icon support. Changes
Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/get-content-type.ts (1)
12-18:⚠️ Potential issue | 🔴 CriticalDo not classify
.doc/.docx/.odt/.rtfastext.
Line 12-Line 17 now routes binary/structured document types to"text". That skips docs conversion (lib/api/documents/process-document.ts, Line 196) and sends incompatible files into the text viewer path (components/view/view-data.tsx, Line 187).🐛 Proposed fix
- case "application/msword": - case "application/vnd.openxmlformats-officedocument.wordprocessingml.document": - case "application/vnd.oasis.opendocument.text": - case "application/rtf": - case "text/rtf": - case "text/plain": - return "text"; + case "application/msword": + case "application/vnd.openxmlformats-officedocument.wordprocessingml.document": + case "application/vnd.oasis.opendocument.text": + case "application/rtf": + case "text/rtf": + return "docs"; + case "text/plain": + return "text";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/get-content-type.ts` around lines 12 - 18, The MIME-to-type mapping in get-content-type.ts incorrectly classifies binary/structured document MIME types as "text"; update the switch in the getContentType (or equivalent) function so that application/msword, application/vnd.openxmlformats-officedocument.wordprocessingml.document, application/vnd.oasis.opendocument.text, application/rtf and text/rtf are not returned as "text" but instead returned as "document" (keeping text/plain as "text"); this will route those files into the document conversion flow (process-document.ts) and the correct viewer path (view-data.tsx).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/documents/preview-viewers/preview-text-viewer.tsx`:
- Around line 178-196: The icon-only pagination Buttons for navigating pages
(the Button components that call goToPreviousPage and goToNextPage and are
disabled based on currentPage/numPages) lack accessible names; add appropriate
aria-label attributes (e.g., aria-label="Previous page" and aria-label="Next
page") or aria-labelledby that include current/total context as needed so screen
readers can announce their purpose, ensuring the labels are present on the
Button elements that wrap ChevronLeftIcon and ChevronRightIcon.
- Around line 64-87: When a new file is loaded the effect's fetchText function
doesn't clear prior viewer state, so stale error/pages/currentPage can leak;
inside the useEffect before starting the async fetch (in the fetchText start or
immediately when file changes) call setError(null), setPages([]) and
setCurrentPage(0) and then setLoading(true) so the viewer is reset for the new
file; update the fetchText/useEffect block (references: useEffect, fetchText,
setError, setPages, setCurrentPage, setLoading, paginateText, file) to perform
these resets before awaiting fetch and keep the existing cancelled checks.
In `@components/view/viewer/text-viewer.tsx`:
- Around line 145-162: Before starting the fetch in the text loading routine,
clear any previous error so a past failure doesn't block future successful
loads; specifically, call setError(null) (or setError("") depending on state
type) right after setLoading(true) and before awaiting fetch in the async loader
that uses setLoading, fetch(file), paginateText, setPages and setPageNumber,
ensuring the cancelled check remains in place.
- Around line 331-346: The useEffect that defines removeQueryParams reads
router.query on mount before Next.js hydration, so add a guard at the top: if
(!router.isReady) return; then perform the existing logic; ensure the effect
depends on router.isReady and dataroomId (and router.query/router.asPath or
router as needed) instead of an empty array so it reruns when hydration
completes; keep the helper removeQueryParams and the router.replace call as-is
but trigger only after router.isReady and dataroomId changes are evaluated.
In `@lib/utils/text-pagination.ts`:
- Around line 74-76: The paginateText function currently reads options into
charsPerLine, linesPerPage and tabSize without validation, which allows
charsPerLine <= 0 or linesPerPage <= 0 and can cause non-terminating loops; fix
by validating and normalizing those values after they are assigned (in the block
that sets charsPerLine, linesPerPage, tabSize) — coerce any non-finite or <=0
charsPerLine and linesPerPage to the corresponding TEXT_PAGE_DEFAULTS values (or
throw a clear ArgumentError) and ensure tabSize is a positive integer as well so
the pagination loop in paginateText always advances.
---
Outside diff comments:
In `@lib/utils/get-content-type.ts`:
- Around line 12-18: The MIME-to-type mapping in get-content-type.ts incorrectly
classifies binary/structured document MIME types as "text"; update the switch in
the getContentType (or equivalent) function so that application/msword,
application/vnd.openxmlformats-officedocument.wordprocessingml.document,
application/vnd.oasis.opendocument.text, application/rtf and text/rtf are not
returned as "text" but instead returned as "document" (keeping text/plain as
"text"); this will route those files into the document conversion flow
(process-document.ts) and the correct viewer path (view-data.tsx).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d5729cb-7648-4168-8054-a970cc14c22b
📒 Files selected for processing (10)
app/api/views/route.tscomponents/documents/preview-viewers/preview-text-viewer.tsxcomponents/documents/preview-viewers/preview-viewer.tsxcomponents/view/view-data.tsxcomponents/view/viewer/text-viewer.tsxlib/constants.tslib/utils/get-content-type.tslib/utils/get-file-icon.tsxlib/utils/text-pagination.tspages/api/teams/[teamId]/documents/[id]/preview-data.ts
| useEffect(() => { | ||
| if (!file) return; | ||
| let cancelled = false; | ||
| async function fetchText() { | ||
| try { | ||
| setLoading(true); | ||
| const response = await fetch(file!); | ||
| if (!response.ok) throw new Error("Failed to load text file"); | ||
| const text = await response.text(); | ||
| if (cancelled) return; | ||
| setPages(paginateText(text)); | ||
| } catch (err) { | ||
| if (!cancelled) { | ||
| setError(err instanceof Error ? err.message : "Failed to load file"); | ||
| } | ||
| } finally { | ||
| if (!cancelled) setLoading(false); | ||
| } | ||
| } | ||
| fetchText(); | ||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [file]); |
There was a problem hiding this comment.
Reset viewer state before loading a new file.
Line 64-Line 87 does not clear previous error/pages or reset currentPage, so stale state can leak across file changes.
💡 Proposed fix
useEffect(() => {
if (!file) return;
let cancelled = false;
async function fetchText() {
try {
+ setError(null);
+ setPages(null);
+ setCurrentPage(1);
setLoading(true);
const response = await fetch(file!);
if (!response.ok) throw new Error("Failed to load text file");
const text = await response.text();
if (cancelled) return;
setPages(paginateText(text));📝 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.
| useEffect(() => { | |
| if (!file) return; | |
| let cancelled = false; | |
| async function fetchText() { | |
| try { | |
| setLoading(true); | |
| const response = await fetch(file!); | |
| if (!response.ok) throw new Error("Failed to load text file"); | |
| const text = await response.text(); | |
| if (cancelled) return; | |
| setPages(paginateText(text)); | |
| } catch (err) { | |
| if (!cancelled) { | |
| setError(err instanceof Error ? err.message : "Failed to load file"); | |
| } | |
| } finally { | |
| if (!cancelled) setLoading(false); | |
| } | |
| } | |
| fetchText(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [file]); | |
| useEffect(() => { | |
| if (!file) return; | |
| let cancelled = false; | |
| async function fetchText() { | |
| try { | |
| setError(null); | |
| setPages(null); | |
| setCurrentPage(1); | |
| setLoading(true); | |
| const response = await fetch(file!); | |
| if (!response.ok) throw new Error("Failed to load text file"); | |
| const text = await response.text(); | |
| if (cancelled) return; | |
| setPages(paginateText(text)); | |
| } catch (err) { | |
| if (!cancelled) { | |
| setError(err instanceof Error ? err.message : "Failed to load file"); | |
| } | |
| } finally { | |
| if (!cancelled) setLoading(false); | |
| } | |
| } | |
| fetchText(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [file]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/documents/preview-viewers/preview-text-viewer.tsx` around lines 64
- 87, When a new file is loaded the effect's fetchText function doesn't clear
prior viewer state, so stale error/pages/currentPage can leak; inside the
useEffect before starting the async fetch (in the fetchText start or immediately
when file changes) call setError(null), setPages([]) and setCurrentPage(0) and
then setLoading(true) so the viewer is reset for the new file; update the
fetchText/useEffect block (references: useEffect, fetchText, setError, setPages,
setCurrentPage, setLoading, paginateText, file) to perform these resets before
awaiting fetch and keep the existing cancelled checks.
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="absolute left-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30" | ||
| onClick={goToPreviousPage} | ||
| disabled={currentPage <= 1} | ||
| > | ||
| <ChevronLeftIcon className="h-6 w-6" /> | ||
| </Button> | ||
|
|
||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| className="absolute right-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30" | ||
| onClick={goToNextPage} | ||
| disabled={currentPage >= numPages} | ||
| > | ||
| <ChevronRightIcon className="h-6 w-6" /> | ||
| </Button> |
There was a problem hiding this comment.
Add accessible names to icon-only pagination buttons.
Line 178 and Line 188 render icon-only controls without aria-label, which makes page navigation unclear for assistive tech users.
♿ Proposed fix
<Button
variant="ghost"
size="icon"
+ aria-label="Previous page"
+ title="Previous page"
className="absolute left-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30"
onClick={goToPreviousPage}
disabled={currentPage <= 1}
>
<ChevronLeftIcon className="h-6 w-6" />
</Button>
<Button
variant="ghost"
size="icon"
+ aria-label="Next page"
+ title="Next page"
className="absolute right-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30"
onClick={goToNextPage}
disabled={currentPage >= numPages}
>
<ChevronRightIcon className="h-6 w-6" />
</Button>📝 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 | |
| variant="ghost" | |
| size="icon" | |
| className="absolute left-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30" | |
| onClick={goToPreviousPage} | |
| disabled={currentPage <= 1} | |
| > | |
| <ChevronLeftIcon className="h-6 w-6" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| className="absolute right-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30" | |
| onClick={goToNextPage} | |
| disabled={currentPage >= numPages} | |
| > | |
| <ChevronRightIcon className="h-6 w-6" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| aria-label="Previous page" | |
| title="Previous page" | |
| className="absolute left-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30" | |
| onClick={goToPreviousPage} | |
| disabled={currentPage <= 1} | |
| > | |
| <ChevronLeftIcon className="h-6 w-6" /> | |
| </Button> | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| aria-label="Next page" | |
| title="Next page" | |
| className="absolute right-2 top-1/2 z-50 h-10 w-10 -translate-y-1/2 rounded-full bg-black/20 text-white hover:bg-black/40 hover:text-white disabled:opacity-30" | |
| onClick={goToNextPage} | |
| disabled={currentPage >= numPages} | |
| > | |
| <ChevronRightIcon className="h-6 w-6" /> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/documents/preview-viewers/preview-text-viewer.tsx` around lines
178 - 196, The icon-only pagination Buttons for navigating pages (the Button
components that call goToPreviousPage and goToNextPage and are disabled based on
currentPage/numPages) lack accessible names; add appropriate aria-label
attributes (e.g., aria-label="Previous page" and aria-label="Next page") or
aria-labelledby that include current/total context as needed so screen readers
can announce their purpose, ensuring the labels are present on the Button
elements that wrap ChevronLeftIcon and ChevronRightIcon.
| setLoading(true); | ||
| const response = await fetch(file); | ||
| if (!response.ok) throw new Error("Failed to load text file"); | ||
| const text = await response.text(); | ||
| if (cancelled) return; | ||
| const paginatedPages = paginateText(text); | ||
| setPages(paginatedPages); | ||
|
|
||
| const initialPage = | ||
| pageQuery >= 1 && pageQuery <= paginatedPages.length ? pageQuery : 1; | ||
| setPageNumber(initialPage); | ||
| } catch (err) { | ||
| if (!cancelled) { | ||
| setError(err instanceof Error ? err.message : "Failed to load file"); | ||
| } | ||
| } finally { | ||
| if (!cancelled) setLoading(false); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "text-viewer.tsx" --exec wc -l {}Repository: mfts/papermark
Length of output: 169
🏁 Script executed:
fd -t f "text-viewer.tsx" -x cat -n {}Repository: mfts/papermark
Length of output: 28880
🏁 Script executed:
fd -t f "text-viewer.tsx" -x sed -n '470,490p' {}Repository: mfts/papermark
Length of output: 591
Reset error before each load to prevent sticky error UI blocking successful retries.
When a fetch fails, error is set at line 158. If the fetch is retried (file prop changes), the error state is never cleared, so line 479's condition error || !pages remains true even if the retry succeeds. The error UI renders instead of the content.
Proposed fix
useEffect(() => {
let cancelled = false;
async function fetchText() {
try {
setLoading(true);
+ setError(null);
const response = await fetch(file);
if (!response.ok) throw new Error("Failed to load text file");
const text = await response.text();
if (cancelled) return;
const paginatedPages = paginateText(text);📝 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.
| setLoading(true); | |
| const response = await fetch(file); | |
| if (!response.ok) throw new Error("Failed to load text file"); | |
| const text = await response.text(); | |
| if (cancelled) return; | |
| const paginatedPages = paginateText(text); | |
| setPages(paginatedPages); | |
| const initialPage = | |
| pageQuery >= 1 && pageQuery <= paginatedPages.length ? pageQuery : 1; | |
| setPageNumber(initialPage); | |
| } catch (err) { | |
| if (!cancelled) { | |
| setError(err instanceof Error ? err.message : "Failed to load file"); | |
| } | |
| } finally { | |
| if (!cancelled) setLoading(false); | |
| } | |
| setLoading(true); | |
| setError(null); | |
| const response = await fetch(file); | |
| if (!response.ok) throw new Error("Failed to load text file"); | |
| const text = await response.text(); | |
| if (cancelled) return; | |
| const paginatedPages = paginateText(text); | |
| setPages(paginatedPages); | |
| const initialPage = | |
| pageQuery >= 1 && pageQuery <= paginatedPages.length ? pageQuery : 1; | |
| setPageNumber(initialPage); | |
| } catch (err) { | |
| if (!cancelled) { | |
| setError(err instanceof Error ? err.message : "Failed to load file"); | |
| } | |
| } finally { | |
| if (!cancelled) setLoading(false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/view/viewer/text-viewer.tsx` around lines 145 - 162, Before
starting the fetch in the text loading routine, clear any previous error so a
past failure doesn't block future successful loads; specifically, call
setError(null) (or setError("") depending on state type) right after
setLoading(true) and before awaiting fetch in the async loader that uses
setLoading, fetch(file), paginateText, setPages and setPageNumber, ensuring the
cancelled check remains in place.
| useEffect(() => { | ||
| const removeQueryParams = (queries: string[]) => { | ||
| const currentQuery = { ...router.query }; | ||
| const currentPath = router.asPath.split("?")[0]; | ||
| queries.map((query) => delete currentQuery[query]); | ||
| router.replace( | ||
| { pathname: currentPath, query: currentQuery }, | ||
| undefined, | ||
| { shallow: true }, | ||
| ); | ||
| }; | ||
|
|
||
| if (!dataroomId && router.query.token) { | ||
| removeQueryParams(["token", "email", "domain", "slug", "linkId"]); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
fd text-viewer.tsx --type f -exec wc -l {} \;Repository: mfts/papermark
Length of output: 244
🏁 Script executed:
# Check the file content around the specified lines
fd text-viewer.tsx --type f -exec head -350 {} \; | tail -100Repository: mfts/papermark
Length of output: 288
🏁 Script executed:
# Get a better view of the imports and overall structure
fd text-viewer.tsx --type f -exec head -50 {} \;Repository: mfts/papermark
Length of output: 288
🏁 Script executed:
# Search for router imports and usage in this file
fd text-viewer.tsx --type f -exec grep -n "router\|useRouter\|import" {} \;Repository: mfts/papermark
Length of output: 288
🏁 Script executed:
# Find the file
fd "text-viewer.tsx"Repository: mfts/papermark
Length of output: 157
🏁 Script executed:
# Once we know the path, read the full file to understand context
git ls-files | grep -E "text-viewer\.tsx"Repository: mfts/papermark
Length of output: 157
🏁 Script executed:
# Read the file to understand its structure and the specific lines
cat -n components/view/viewer/text-viewer.tsx | head -100Repository: mfts/papermark
Length of output: 3478
🏁 Script executed:
# Read the specific lines mentioned in the review (331-346) and surrounding context
cat -n components/view/viewer/text-viewer.tsx | sed -n '300,360p'Repository: mfts/papermark
Length of output: 2047
🏁 Script executed:
# Check imports at the top of the file
cat -n components/view/viewer/text-viewer.tsx | sed -n '1,50p'Repository: mfts/papermark
Length of output: 1834
🌐 Web query:
Next.js router.isReady hydration query parameter timing
💡 Result:
In the Pages Router (next/router), router.query (and asPath) can be missing/empty on the first render because the page may be pre-rendered (static optimization) and Next.js only fills in those router fields client-side after hydration. The router exposes router.isReady to tell you when those fields have been updated and are safe to read. [1]
Practical pattern (query-string + dynamic params):
import { useEffect } from "react";
import { useRouter } from "next/router";
export default function Page() {
const router = useRouter();
useEffect(() => {
if (!router.isReady) return; // wait for hydration update
const { foo, slug } = router.query; // now stable
// ...
}, [router.isReady, router.query]);
return null;
}Notes:
- Next.js docs explicitly warn to avoid using
asPathuntilisReadyis true, and thatisReadyis meant to be used insideuseEffect, not for server-conditional rendering. [1] - The same “
null/empty on initial render, then populated after hydration when the router is ready” behavior is also documented foruseParams()on statically optimized pages. [2] - If you need values on the server render (no hydration timing), read them in
getServerSideProps(or in the App Router, preferuseSearchParams()/useParams()fromnext/navigation). [1][2]
Sources:
[1] Next.js Docs — useRouter (isReady, query, asPath hydration timing) (nextjs.org)
[2] Next.js Docs — useParams (“returns null on initial render for statically optimized pages; updated after hydration once router is ready”) (nextjs.org)
Citations:
- 1: https://nextjs.org/docs/pages/api-reference/functions/use-router
- 2: https://nextjs.org/docs/pages/api-reference/functions/use-params
🌐 Web query:
Next.js useRouter router.query undefined before hydration
💡 Result:
In the Pages Router (next/router), router.query can be {} / have undefined fields on the first render because during pre-rendering (Automatic Static Optimization / SSG) Next.js doesn’t yet have access to the browser URL; it fills query after hydration, causing a second render. Next.js documents this behavior and recommends using router.isReady to know when query is usable. [1][2]
Fix (recommended): wait for router.isReady (typically in useEffect)
import { useEffect } from "react";
import { useRouter } from "next/router";
export default function Page() {
const router = useRouter();
useEffect(() => {
if (!router.isReady) return;
const { id } = router.query; // now reliable
// ...use id
}, [router.isReady, router.query]);
return null;
}isReady is specifically meant to indicate when router fields (like query / asPath) have been updated client-side. [1][2]
If you need the param on the first render (server/HTML)
Use data fetching so the param is available on the server (e.g., getServerSideProps, or getStaticProps for dynamic routes), instead of relying on useRouter() during prerender. [2]
If you’re on the App Router (app/)
useRouter from next/navigation doesn’t expose query; use:
useParams()for dynamic segments, anduseSearchParams()for?key=valuequery strings. [1]
Sources:
[1] Next.js useRouter (Pages Router) API reference (updated Feb 11, 2026)
[2] Next.js Automatic Static Optimization docs (explains empty query before hydration and using isReady)
Add router.isReady guard before accessing router fields.
The effect runs on mount with an empty dependency array. In Next.js Pages Router, router.query is empty until hydration completes. This means the condition !dataroomId && router.query.token always fails on first render, and the effect never reruns, leaving token/email parameters in the URL.
Add the check at the top of the effect and update dependencies per the Next.js documentation pattern:
Fix
useEffect(() => {
+ if (!router.isReady) return;
+
const removeQueryParams = (queries: string[]) => {
const currentQuery = { ...router.query };
const currentPath = router.asPath.split("?")[0];
- queries.map((query) => delete currentQuery[query]);
+ queries.forEach((query) => delete currentQuery[query]);
router.replace(
{ pathname: currentPath, query: currentQuery },
undefined,
{ shallow: true },
);
};
if (!dataroomId && router.query.token) {
removeQueryParams(["token", "email", "domain", "slug", "linkId"]);
}
- }, []);
+ }, [router.isReady, router.query, router.asPath, dataroomId]);📝 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.
| useEffect(() => { | |
| const removeQueryParams = (queries: string[]) => { | |
| const currentQuery = { ...router.query }; | |
| const currentPath = router.asPath.split("?")[0]; | |
| queries.map((query) => delete currentQuery[query]); | |
| router.replace( | |
| { pathname: currentPath, query: currentQuery }, | |
| undefined, | |
| { shallow: true }, | |
| ); | |
| }; | |
| if (!dataroomId && router.query.token) { | |
| removeQueryParams(["token", "email", "domain", "slug", "linkId"]); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| if (!router.isReady) return; | |
| const removeQueryParams = (queries: string[]) => { | |
| const currentQuery = { ...router.query }; | |
| const currentPath = router.asPath.split("?")[0]; | |
| queries.forEach((query) => delete currentQuery[query]); | |
| router.replace( | |
| { pathname: currentPath, query: currentQuery }, | |
| undefined, | |
| { shallow: true }, | |
| ); | |
| }; | |
| if (!dataroomId && router.query.token) { | |
| removeQueryParams(["token", "email", "domain", "slug", "linkId"]); | |
| } | |
| }, [router.isReady, router.query, router.asPath, dataroomId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/view/viewer/text-viewer.tsx` around lines 331 - 346, The useEffect
that defines removeQueryParams reads router.query on mount before Next.js
hydration, so add a guard at the top: if (!router.isReady) return; then perform
the existing logic; ensure the effect depends on router.isReady and dataroomId
(and router.query/router.asPath or router as needed) instead of an empty array
so it reruns when hydration completes; keep the helper removeQueryParams and the
router.replace call as-is but trigger only after router.isReady and dataroomId
changes are evaluated.
| const charsPerLine = options?.charsPerLine ?? TEXT_PAGE_DEFAULTS.charsPerLine; | ||
| const linesPerPage = options?.linesPerPage ?? TEXT_PAGE_DEFAULTS.linesPerPage; | ||
| const tabSize = options?.tabSize ?? TEXT_PAGE_DEFAULTS.tabSize; |
There was a problem hiding this comment.
Validate pagination options to prevent non-terminating loops.
paginateText accepts caller overrides but does not guard invalid values. If charsPerLine <= 0 (Line 40) or linesPerPage <= 0 (Line 93), pagination can hang indefinitely.
Proposed fix
export function paginateText(
rawText: string,
options?: PaginateTextOptions,
): string[][] {
const charsPerLine = options?.charsPerLine ?? TEXT_PAGE_DEFAULTS.charsPerLine;
const linesPerPage = options?.linesPerPage ?? TEXT_PAGE_DEFAULTS.linesPerPage;
const tabSize = options?.tabSize ?? TEXT_PAGE_DEFAULTS.tabSize;
+
+ if (!Number.isInteger(charsPerLine) || charsPerLine < 1) {
+ throw new Error("charsPerLine must be a positive integer");
+ }
+ if (!Number.isInteger(linesPerPage) || linesPerPage < 1) {
+ throw new Error("linesPerPage must be a positive integer");
+ }
+ if (!Number.isInteger(tabSize) || tabSize < 1) {
+ throw new Error("tabSize must be a positive integer");
+ }
const normalizedText = rawText.replace(/\r\n/g, "\n").replace(/\r/g, "\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/utils/text-pagination.ts` around lines 74 - 76, The paginateText function
currently reads options into charsPerLine, linesPerPage and tabSize without
validation, which allows charsPerLine <= 0 or linesPerPage <= 0 and can cause
non-terminating loops; fix by validating and normalizing those values after they
are assigned (in the block that sets charsPerLine, linesPerPage, tabSize) —
coerce any non-finite or <=0 charsPerLine and linesPerPage to the corresponding
TEXT_PAGE_DEFAULTS values (or throw a clear ArgumentError) and ensure tabSize is
a positive integer as well so the pagination loop in paginateText always
advances.
2 participants
Adds a native text file viewer with client-side pagination and canvas rendering for enhanced security and consistent display.
The viewer renders text files on an HTML5 canvas, preventing text selection and copying. It uses a fixed 80x52 character grid with word-wrapping for consistent page breaks across devices. All pagination and rendering occur client-side, with the raw
.txtfile stored as-is, andtext/plainfiles are now correctly identified as"text"type, bypassing server-side document conversion.Summary by CodeRabbit