Skip to content

Add PDF streaming viewer and token caching #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
akashroy8210 wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Add PDF streaming viewer and token caching #174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions client/src/screens/browse/components/file-display/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { getThumbnail } from "../../../../api/File";
import clientRoot from "../../../../api/server";
import capitalise from "../../../../utils/capitalise.js";
import Share from "../../../share";
import API_BASE_URL from "../../../../api/server";
import { verifyFile, unverifyFile } from "../../../../api/File";
import {
RemoveFileFromFolder,
Expand All @@ -18,12 +19,14 @@ import { getFileDownloadLink } from "../../../../api/File";
import { fetchFolder } from "../../../../api/Folder.js";

const FileDisplay = ({ file, path, code, isMobileView = false }) => {
console.log(file._id);

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these leftover debug logs.


const user = useSelector((state) => state.user?.user);
const fileSize = formatFileSize(file.size);
const fileType = formatFileType(file.name);
const [showDialog, setShowDialog] = useState(false);
const [dialogType, setDialogType] = useState("verify");
const [onConfirmAction, setOnConfirmAction] = useState(() => () => {});
const [onConfirmAction, setOnConfirmAction] = useState(() => () => { });
const [isProcessing, setIsProcessing] = useState(false);

let name = file.name;
Expand Down Expand Up @@ -96,7 +99,10 @@ const FileDisplay = ({ file, path, code, isMobileView = false }) => {
toast.error("Please login to preview file.");
return;
}
window.open(preview_url, "_blank");
window.open(

@RsbhThakur RsbhThakur Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

`${API_BASE_URL}/api/contribution/view/${file._id}`,
"_blank"
);
};


Expand Down Expand Up @@ -144,9 +150,8 @@ const FileDisplay = ({ file, path, code, isMobileView = false }) => {

return (
<div
className={`file-display ${
user?.isBR ? (file.isVerified ? "verified" : "unverified") : ""
}`}
className={`file-display ${user?.isBR ? (file.isVerified ? "verified" : "unverified") : ""
}`}
>
<img
src={thumbnailUrl}
Expand Down
1 change: 1 addition & 0 deletions server/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ app.use((err, req, res, next) => {
logger.error(
{
message: err.message,
microsoftResponse: err.response?.data,
route: req.originalUrl,
method: req.method,
},
Expand Down
61 changes: 58 additions & 3 deletions server/modules/contribution/contribution.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import AppError from "../../utils/appError.js";
import validatePayload from "../../utils/validate.js";
import UploadFile from "../../services/UploadFile.js";
import fs from "fs";
import { FolderModel } from "../course/course.model.js";
import { FolderModel, FileModel } from "../course/course.model.js";
import logger from "../../utils/logger.js";
import { normalizeCourseCode, getCourseCodeCaseInsensitiveRegex } from "../../utils/course.js";
import { getAccessToken } from "../onedrive/onedrive.controller.js";
import axios from "axios";

async function ContributionCreation(contributionId, data) {
const existingContribution = await Contribution.findOne({ contributionId });
Expand Down Expand Up @@ -43,14 +45,15 @@ async function HandleFileToDB(contributionId, fileId) {

async function GetAllContributions(req, res, next) {
const allContributions = await Contribution.find({});
console.log(allContributions);

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these leftover debug logs

res.json(allContributions);
}

async function HandleFileUpload(req, res, next) {
logger.info("Handling File Upload");
const contributionId = req.headers["contribution-id"];
const files = req.files;

console.log(files.length)

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these leftover debug logs to keep the production logs clean

if (!files || files.length === 0) {
return res.status(400).json({ error: "No files were uploaded" });
}
Expand Down Expand Up @@ -157,12 +160,64 @@ async function GetBrContribution(req, res, next) {
}
}

async function viewFile(req, res, next) {

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
const { id } = req.params;

console.time("file is fetched")

const file = await FileModel.findById(id);

console.timeEnd("file is fetched")

if (!file) {
console.log("file not found")
return res.status(404).json({ message: "File not found" });
}

console.time("access token fetched")

const accessToken = await getAccessToken();
if (!accessToken) {
return res.status(500).json({ message: "Access token not found" });
}

console.timeEnd("access token fetched")

console.time("graph request")
const response = await axios.get(`https://graph.microsoft.com/v1.0/me/drive/items/${file.fileId}/content`, {
headers: {
Authorization: ` Bearer ${accessToken}`
},
responseType: "stream"
})
if (!response) {
return res.status(500).json({ message: "File not found" });
}
console.timeEnd("graph request")

console.time("res sent to client")
res.setHeader("Content-Type", "application/pdf")

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

console.timeEnd("res sent to client")

console.time("stream finish")
response.data.on("end", () => {
console.timeEnd("stream finish");
});
response.data.pipe(res)

} catch (error) {
next(error);
}
}

export default {
GetAllContributions,
CreateNewContribution,
HandleFileUpload,
GetMyContributions,
DeleteContribution,
GetContributionsUpdatedSince,
GetBrContribution
GetBrContribution,
viewFile
};
2 changes: 1 addition & 1 deletion server/modules/contribution/contribution.routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ router.post("/upload", upload.array("file"), catchAsync(ContributionController.H
// router.get("/:id", catchAsync(ContributionController.CreateNewContribution));
router.post("/updated", catchAsync(ContributionController.GetContributionsUpdatedSince));
router.post("/br",isAuthenticated, ContributionController.GetBrContribution)

router.get('/view/:id',catchAsync(ContributionController.viewFile))
export default router;
33 changes: 32 additions & 1 deletion server/modules/onedrive/onedrive.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,14 +319,45 @@ async function visitFile(file, currCourse) {
return NewFile._id;
}

// export async function getAccessToken() {
// let data;
// if (fs.existsSync("./onedrive-refresh-token.token")) {
// data = await refreshAccessToken();
// } else {
// data = await generateAccessToken();
// }
// return data.access_token;
// }
let cachedAccessToken = null;
let tokenExpiry = 0;

export async function getAccessToken() {

if (
cachedAccessToken &&
Date.now() < tokenExpiry
) {
console.log("Using cached token");

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these debug logs to keep the production console clean.

return cachedAccessToken;
}

console.log("Fetching fresh token");

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these debug logs to keep the production console clean.


let data;

if (fs.existsSync("./onedrive-refresh-token.token")) {
data = await refreshAccessToken();

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
data = await generateAccessToken();
}
return data.access_token;

cachedAccessToken = data.access_token;

tokenExpiry =
Date.now() +
(data.expires_in - 60) * 1000;

return cachedAccessToken;
}

async function refreshAccessToken() {
Expand Down
2 changes: 2 additions & 0 deletions server/services/UploadFile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ async function createUploadSession(folderId, fileName) {

async function UploadFile(contributionId, filePath, fileName) {
const folderId = parent_item_id;
console.log("Folder ID:", folderId);

@RsbhThakur RsbhThakur Jul 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this leftover console logs on line 79 and 80

console.log("ONEDRIVE_FOLDER_ID", process.env.ONEDRIVE_FOLDER_ID);
const session = await createUploadSession(folderId, fileName);
if (!session?.url) {
logger.error("Error uploading!");
Expand Down