Skip to content
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

chore: fix lint issues in src/share #1384

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from
Draft

chore: fix lint issues in src/share #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bdc8291
chore(share): use safeExtractMessageAndStackFromError to get rid of "…
pano9000 Mar 10, 2025
c2b75a6
chore(share): fix @typescript-eslint/no-unused-vars for "next"
pano9000 Mar 10, 2025
cd9d903
chore(share): fix @typescript-eslint/no-unused-vars for unused note v…
pano9000 Mar 10, 2025
c2aae45
chore(share): fix no-unused-vars and prefer-const lint issues
pano9000 Mar 10, 2025
2a5ac80
chore(utils/safeExtractMessageAndStackFromError): add explicit return…
pano9000 Mar 10, 2025
a5a66a1
chore(share): fix tsc nagging about svg not existing on unknown
pano9000 Mar 10, 2025
ae1ef55
chore(share): remove unused "_next"
pano9000 Mar 15, 2025
73305a5
chore(share): remove unused args
pano9000 Mar 15, 2025
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
2 changes: 1 addition & 1 deletion src/services/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ export function processStringOrBuffer(data: string | Buffer | null) {
}
}

export function safeExtractMessageAndStackFromError(err: unknown) {
export function safeExtractMessageAndStackFromError(err: unknown): [errMessage: string, errStack: string | undefined] {
return (err instanceof Error) ? [err.message, err.stack] as const : ["Unknown Error", undefined] as const;
}

Expand Down
6 changes: 4 additions & 2 deletions src/share/routes.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { beforeAll, beforeEach, describe, expect, it } from "vitest";
import supertest from "supertest";
import { initializeTranslations } from "../services/i18n.js";
import type { Application, Request, Response, NextFunction } from "express";
import { safeExtractMessageAndStackFromError } from "../services/utils.js";

let app: Application;

Expand All @@ -11,8 +12,9 @@ describe("Share API test", () => {
beforeAll(async () => {
initializeTranslations();
app = (await import("../app.js")).default;
app.use((err: any, req: Request, res: Response, next: NextFunction) => {
if (err.message.includes("Cannot set headers after they are sent to the client")) {
app.use((err: unknown, req: Request, res: Response, next: NextFunction) => {
const [errMessage] = safeExtractMessageAndStackFromError(err)
if (errMessage.includes("Cannot set headers after they are sent to the client")) {
cannotSetHeadersCount++;
}

Expand Down
37 changes: 22 additions & 15 deletions src/share/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import log from "../services/log.js";
import type SNote from "./shaca/entities/snote.js";
import type SBranch from "./shaca/entities/sbranch.js";
import type SAttachment from "./shaca/entities/sattachment.js";
import utils from "../services/utils.js";
import utils, { safeExtractMessageAndStackFromError } from "../services/utils.js";
import options from "../services/options.js";

function getSharedSubTreeRoot(note: SNote): { note?: SNote; branch?: SBranch } {
Expand Down Expand Up @@ -115,7 +115,14 @@ function renderImageAttachment(image: SNote, res: Response, attachmentName: stri
svgString = content;
} else {
// backwards compatibility, before attachments, the SVG was stored in the main note content as a separate key
const contentSvg = image.getJsonContentSafely()?.svg;
const possibleSvgContent = image.getJsonContentSafely();

const contentSvg = (typeof possibleSvgContent === "object"
&& possibleSvgContent !== null
&& "svg" in possibleSvgContent
&& typeof possibleSvgContent.svg === "string")
? possibleSvgContent.svg
: null;
Comment on lines +120 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite hard to read, I would prefer extracting it in a method and using if statements and writing a test for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume you mean moving this as method into SNote class?

something like this?
It still looks ugly, but not much we can do about it I guess.
As I've noted in my commit message, using a schema validator like zod would make life a lot easier for these types of situation (but that would be something for the future, not this PR I'd say).

snote.ts

        getSvgContent() {
            const possibleSvgContent = this.getJsonContentSafely();

            return (typeof possibleSvgContent === "object"
                && possibleSvgContent !== null
                && "svg" in possibleSvgContent
                && typeof possibleSvgContent.svg === "string")
                    ? possibleSvgContent.svg
                    : null;
        }

routes.ts

        const contentSvg = image.getSvgContent();

Copy link
Contributor

Choose a reason for hiding this comment

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

@pano9000 , seems fine, but I would still break it into multiple statements, something along the lines of:

getSvgContent() {
    const possibleSvgContent = this.getJsonContentSafely();
    if (!possibleSvgContent || typeof possibleSvgContent !== "object") {
        return null;
    }

    if (!("svg" in possibleSvgContent) || typeof possibleSvgContent.svg !== "string") {
        return;
    }


    return possibleSvgContent.svg;
}


if (contentSvg) {
svgString = contentSvg;
Expand Down Expand Up @@ -183,8 +190,9 @@ function register(router: Router) {
res.send(ejsResult);
useDefaultView = false; // Rendering went okay, don't use default view
}
} catch (e: any) {
log.error(`Rendering user provided share template (${templateId}) threw exception ${e.message} with stacktrace: ${e.stack}`);
} catch (e: unknown) {
const [errMessage, errStack] = safeExtractMessageAndStackFromError(e);
log.error(`Rendering user provided share template (${templateId}) threw exception ${errMessage} with stacktrace: ${errStack}`);
}
}
}
Expand All @@ -194,7 +202,7 @@ function register(router: Router) {
}
}

router.get("/share/", (req, res, next) => {
router.get("/share/", (req, res) => {
if (req.path.substr(-1) !== "/") {
res.redirect("../share/");
return;
Expand All @@ -210,7 +218,7 @@ function register(router: Router) {
renderNote(shaca.shareRootNote, req, res);
});

router.get("/share/:shareId", (req, res, next) => {
router.get("/share/:shareId", (req, res) => {
shacaLoader.ensureLoad();

const { shareId } = req.params;
Expand All @@ -220,7 +228,7 @@ function register(router: Router) {
renderNote(note, req, res);
});

router.get("/share/api/notes/:noteId", (req, res, next) => {
router.get("/share/api/notes/:noteId", (req, res) => {
shacaLoader.ensureLoad();
let note: SNote | boolean;

Expand All @@ -233,7 +241,7 @@ function register(router: Router) {
res.json(note.getPojo());
});

router.get("/share/api/notes/:noteId/download", (req, res, next) => {
router.get("/share/api/notes/:noteId/download", (req, res) => {
shacaLoader.ensureLoad();

let note: SNote | boolean;
Expand All @@ -255,7 +263,7 @@ function register(router: Router) {
});

// :filename is not used by trilium, but instead used for "save as" to assign a human-readable filename
router.get("/share/api/images/:noteId/:filename", (req, res, next) => {
router.get("/share/api/images/:noteId/:filename", (req, res) => {
shacaLoader.ensureLoad();

let image: SNote | boolean;
Expand All @@ -281,7 +289,7 @@ function register(router: Router) {
});

// :filename is not used by trilium, but instead used for "save as" to assign a human-readable filename
router.get("/share/api/attachments/:attachmentId/image/:filename", (req, res, next) => {
router.get("/share/api/attachments/:attachmentId/image/:filename", (req, res) => {
shacaLoader.ensureLoad();

let attachment: SAttachment | boolean;
Expand All @@ -299,7 +307,7 @@ function register(router: Router) {
}
});

router.get("/share/api/attachments/:attachmentId/download", (req, res, next) => {
router.get("/share/api/attachments/:attachmentId/download", (req, res) => {
shacaLoader.ensureLoad();

let attachment: SAttachment | boolean;
Expand All @@ -321,7 +329,7 @@ function register(router: Router) {
});

// used for PDF viewing
router.get("/share/api/notes/:noteId/view", (req, res, next) => {
router.get("/share/api/notes/:noteId/view", (req, res) => {
shacaLoader.ensureLoad();

let note: SNote | boolean;
Expand All @@ -339,19 +347,18 @@ function register(router: Router) {
});

// Used for searching, require noteId so we know the subTreeRoot
router.get("/share/api/notes", (req, res, next) => {
router.get("/share/api/notes", (req, res) => {
shacaLoader.ensureLoad();

const ancestorNoteId = req.query.ancestorNoteId ?? "_share";
let note;

if (typeof ancestorNoteId !== "string") {
res.status(400).json({ message: "'ancestorNoteId' parameter is mandatory." });
return;
}

// This will automatically return if no ancestorNoteId is provided and there is no shareIndex
if (!(note = checkNoteAccess(ancestorNoteId, req, res))) {
if (!checkNoteAccess(ancestorNoteId, req, res)) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/share/shaca/entities/sattachment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class SAttachment extends AbstractShacaEntity {
}
}

let content = row.content;
const content = row.content;

if (this.hasStringContent()) {
return content === null ? "" : content.toString("utf-8");
Expand Down
4 changes: 2 additions & 2 deletions src/share/shaca/entities/snote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class SNote extends AbstractShacaEntity {
}
}

let content = row.content;
const content = row.content;

if (this.hasStringContent()) {
return content === null ? "" : content.toString("utf-8");
Expand Down Expand Up @@ -212,7 +212,7 @@ class SNote extends AbstractShacaEntity {
/**
* @throws Error in case of invalid JSON
*/
getJsonContent(): any | null {
getJsonContent(): unknown | null {
const content = this.getContent();

if (typeof content !== "string" || !content || !content.trim()) {
Expand Down
2 changes: 1 addition & 1 deletion src/share/shaca/shaca_loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function ensureLoad() {

eventService.subscribe(
[eventService.ENTITY_CREATED, eventService.ENTITY_CHANGED, eventService.ENTITY_DELETED, eventService.ENTITY_CHANGE_SYNCED, eventService.ENTITY_DELETE_SYNCED],
({ entityName, entity }) => {
() => {
shaca.reset();
}
);
Expand Down
Loading