Add PDF streaming viewer and token caching#174
Conversation
| import { fetchFolder } from "../../../../api/Folder.js"; | ||
|
|
||
| const FileDisplay = ({ file, path, code, isMobileView = false }) => { | ||
| console.log(file._id); |
There was a problem hiding this comment.
Please remove these leftover debug logs.
| return; | ||
| } | ||
| window.open(preview_url, "_blank"); | ||
| window.open( |
There was a problem hiding this comment.
Critical Bug: This redirects all file types through the new streaming endpoint, but the backend currently hardcodes the response to PDF. This will break the preview for images, .docx, .pptx, and .jpg files.
Please either add a check here to only use this endpoint if the file is a PDF (falling back to preview_url for others) OR update the backend to dynamically send the correct Content-Type.
RsbhThakur
left a comment
RsbhThakur
left a comment
There was a problem hiding this comment.
Many critical issues with this PR. please fix all before merging.
|
|
||
| async function GetAllContributions(req, res, next) { | ||
| const allContributions = await Contribution.find({}); | ||
| console.log(allContributions); |
There was a problem hiding this comment.
Please remove these leftover debug logs
| const contributionId = req.headers["contribution-id"]; | ||
| const files = req.files; | ||
|
|
||
| console.log(files.length) |
There was a problem hiding this comment.
Please remove these leftover debug logs to keep the production logs clean
| console.timeEnd("graph request") | ||
|
|
||
| console.time("res sent to client") | ||
| res.setHeader("Content-Type", "application/pdf") |
There was a problem hiding this comment.
Critical Bug: Because coursehub accepts all file types (images, word documents, etc.), hardcoding the Content-Type to application/pdf will cause the browser to fail when trying to preview non-PDF files.
Please dynamically set the Content-Type based on the file extension (e.g., using a library like mime-types), or restrict this endpoint to only handle PDFs.
| } | ||
| } | ||
|
|
||
| async function viewFile(req, res, next) { |
There was a problem hiding this comment.
There are about 8 leftover console.time, console.timeEnd, and console.log statements throughout this viewFile function. Please remove all performance profiling and debug code before we merge.
| cachedAccessToken && | ||
| Date.now() < tokenExpiry | ||
| ) { | ||
| console.log("Using cached token"); |
There was a problem hiding this comment.
Please remove these debug logs to keep the production console clean.
| return cachedAccessToken; | ||
| } | ||
|
|
||
| console.log("Fetching fresh token"); |
There was a problem hiding this comment.
Please remove these debug logs to keep the production console clean.
| let data; | ||
|
|
||
| if (fs.existsSync("./onedrive-refresh-token.token")) { | ||
| data = await refreshAccessToken(); |
There was a problem hiding this comment.
Optimization: There is a slight race condition here. If a large class of students all try to view a PDF at the exact moment the token expires, the server will concurrently execute refreshAccessToken() dozens of times. Because that function writes to the .token file on disk, these concurrent writes could corrupt the file or cause Microsoft to revoke the token.
Consider wrapping the refresh call in a Promise lock so that concurrent requests just wait for the first refresh to finish.
|
|
||
| async function UploadFile(contributionId, filePath, fileName) { | ||
| const folderId = parent_item_id; | ||
| console.log("Folder ID:", folderId); |
There was a problem hiding this comment.
Please remove this leftover console logs on line 79 and 80
3 participants
Changes
Performance
Observed timings after caching:
User Impact
Users can now open PDFs directly in a new browser tab without being redirected to OneDrive or repeatedly authenticating.