Conversation
9bc3944 to
6bc9d59
Compare
7790fd4 to
a3fbafa
Compare
b07c658 to
f54cb5f
Compare
a5c31c6 to
cb585ce
Compare
767a5e8 to
776f2f3
Compare
b0a1f20 to
050c71d
Compare
a890783 to
c5c3c6b
Compare
9dee7f1 to
9b9e075
Compare
9b9e075 to
3b70a29
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new task system (DB-backed tasks, logs, APIs, and UI pages) and refactors uploads + GLTF processing to run through tasks, while also modernizing server/runtime requirements (Node >=18.17) and adding related UI styling and e2e/test updates.
Changes:
- Add task scheduler/queue/logger infrastructure with migrations, REST endpoints, and UI pages for listing tasks and viewing task trees/logs.
- Refactor scene/zip uploads to use task artifacts + task handlers (upload parsing, zip extraction, GLB inspection/optimization/conversion, doc generation).
- Add new GLTF tooling (gltf-transform + KTX compression pipeline) and supporting utilities/tests; update Dockerfile to bundle
ktx.
Reviewed changes
Copilot reviewed 103 out of 115 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| source/ui/styles/titles.scss | Add positioning/style for a header “caret/back” link element. |
| source/ui/styles/tasks.scss | New shared styles for tasks list/tree/logs views. |
| source/ui/styles/tables.scss | Improve section-contained table layout and add compact/mono column helpers. |
| source/ui/styles/main.scss | Include new tasks stylesheet in main bundle. |
| source/ui/styles/forms.scss | Adjust select sizing in inline form layouts. |
| source/ui/state/uploader.ts | New chunked uploader controller posting to /tasks and uploading task artifacts. |
| source/ui/MainView.ts | Switch UI upload component wiring to new UploadManager and handler imports. |
| source/ui/composants/Size.ts | Change byte formatting API and simplify Size component behavior. |
| source/server/vfs/index.ts | Create uploads/objects/artifacts dirs via VFS accessors at open time. |
| source/server/vfs/helpers/db.ts | Minor debug guard + docstring tweaks + Pool initialization formatting. |
| source/server/vfs/Files.ts | Add copyFile() helper computing hash and hard-linking content to objects. |
| source/server/vfs/Base.ts | Add artifacts dir + task workspace helpers + relative/absolute path helpers. |
| source/server/utils/wrapAsync.ts | Export AsyncRequestHandler type. |
| source/server/utils/templates.ts | Expose template translator (t) for use outside templates. |
| source/server/utils/schema/default.ts | Add explicit schema typing and make default document sync. |
| source/server/utils/languages.ts | Centralize scene/UI language lists and type guards. |
| source/server/utils/gltf/toktx.ts | New KTX2 conversion transform using ktx CLI + sharp resizing. |
| source/server/utils/gltf/obj2gltf.d.ts | Add conservative type definitions for obj2gltf. |
| source/server/utils/gltf/io.ts | Shared glTF-Transform NodeIO with registered extensions/dependencies. |
| source/server/utils/gltf/inspect.ts | New inspector for bounds, faces, textures, extensions. |
| source/server/utils/gltf/inspect.test.ts | Tests for inspector behavior and fixtures. |
| source/server/utils/glTF.ts | Remove legacy custom glTF parsing implementation. |
| source/server/utils/glTF.test.ts | Remove tests for legacy glTF parsing implementation. |
| source/server/utils/format.ts | Add shared formatBytes() + isTimeInterval() helpers. |
| source/server/utils/format.test.ts | Tests for new format utilities. |
| source/server/utils/filetypes.ts | Rework MIME handling; add extension lookup and magic-bytes sniffing. |
| source/server/utils/filetypes.test.ts | Expand MIME/magic-bytes/extension tests. |
| source/server/utils/exec.ts | Add spawn wrappers and ktx --version helper. |
| source/server/utils/exec.test.ts | Tests for exec wrapper behavior. |
| source/server/utils/errors.ts | Add more HTTP error subclasses and rename RangeNotSatisfiable class. |
| source/server/utils/config.ts | Add scripts_dir config value. |
| source/server/utils/archives.ts | Add archive entry parsing helpers for scene-scoped ZIPs. |
| source/server/utils/archives.test.ts | Tests for archive path parsing. |
| source/server/tsconfig.json | Update TS lib/types and include configuration. |
| source/server/tests-common.ts | Update integration harness to use new createService() lifecycle. |
| source/server/templates/upload.hbs | Replace old upload form with UploadManager UI and created-scenes report. |
| source/server/templates/tasks.hbs | New tasks list page template. |
| source/server/templates/task.hbs | New task detail page template (tree + logs). |
| source/server/templates/locales/fr.yml | Add/rename i18n keys for tasks and upload updates (FR). |
| source/server/templates/locales/en.yml | Add/rename i18n keys for tasks and upload updates (EN). |
| source/server/tasks/types.ts | Introduce shared task types (definitions, logs, handlers, artifacts). |
| source/server/tasks/scheduler.ts | New async-context-aware scheduler with nested queues and logging. |
| source/server/tasks/queue.ts | New concurrency queue primitive with close/abort semantics. |
| source/server/tasks/queue.test.ts | Tests for queue scheduling/closing behavior. |
| source/server/tasks/logger.ts | Batched DB log writer with AsyncDisposable lifecycle. |
| source/server/tasks/logger.test.ts | Tests for batching + DB insertion + disposal behavior. |
| source/server/tasks/handlers/toGlb.ts | Task handler to convert OBJ → GLB via obj2gltf. |
| source/server/tasks/handlers/toGlb.test.ts | Integration tests for OBJ conversion task. |
| source/server/tasks/handlers/optimizeGlb.ts | Task handler to optimize GLBs (meshopt + KTX texture compression). |
| source/server/tasks/handlers/inspectGlb.ts | Task handler to inspect GLB using shared IO/inspect utilities. |
| source/server/tasks/handlers/index.ts | Export user-invokable task handlers. |
| source/server/tasks/handlers/extractZip.ts | Task handler to extract multi-scene archives with access checks. |
| source/server/tasks/handlers/createDocumentFromFiles.ts | Task handler to generate default scene docs from model metadata. |
| source/server/tasks/handlers/createDocumentFromFiles.test.ts | Tests for document generation handler. |
| source/server/tasks/errors.ts | Serialize/parse task errors for DB storage/transport. |
| source/server/tasks/errors.test.ts | Tests for task error serialization/parsing. |
| source/server/scripts/obj2gltf.py | Add Blender-based conversion utility script. |
| source/server/routes/views/index.ts | Add /ui/tasks, /ui/tasks/:id, and enhanced /ui/upload rendering. |
| source/server/routes/tasks/task/tree/get.ts | Add API to get task tree + logs. |
| source/server/routes/tasks/task/get.ts | Add API to get a single task + logs. |
| source/server/routes/tasks/task/delete.ts | Add API to delete tasks and cleanup workspace. |
| source/server/routes/tasks/task/artifacts/put.ts | Add resumable upload endpoint for task artifact content. |
| source/server/routes/tasks/task/artifacts/put.test.ts | Tests for resumable task artifact upload + parsing. |
| source/server/routes/tasks/task/artifacts/get.ts | Add endpoint to download artifacts for successful tasks. |
| source/server/routes/tasks/post.ts | Add task creation endpoint for user-invokable task handlers. |
| source/server/routes/tasks/index.ts | New router mounting task APIs with access middleware. |
| source/server/routes/services/opensearch.ts | TS typing tweak for hashing body. |
| source/server/routes/scenes/scene/post.ts | Refactor single-scene upload to use task handlers (inspect + doc gen). |
| source/server/routes/scenes/scene/post.test.ts | Update expectations for new upload file location. |
| source/server/routes/scenes/scene/files/get/file.ts | Update renamed RangeNotSatisfiable error class usage. |
| source/server/routes/scenes/scene/files/get/document.ts | TS typing tweak for hashing document body. |
| source/server/routes/scenes/post.ts | Refactor raw ZIP import to run under tasks/workspaces. |
| source/server/routes/scenes/post.test.ts | Update expected response shape/error behavior for ZIP import. |
| source/server/routes/scenes/index.ts | Add JSON body parser middleware (route-level). |
| source/server/routes/index.ts | Refactor server creation to accept injected services; mount /tasks router. |
| source/server/package.json | Bump Node engine + add glTF/task-related dependencies. |
| source/server/migrations/005-tasks.sql | Add DB schema for tasks and task logs. |
| source/server/integration.test.ts | Update integration expectations for new upload paths/doc location. |
| source/server/index.ts | Switch server boot to createService() wrapper. |
| source/server/create.ts | New service builder composing DB/VFS/UserManager/TaskScheduler + app. |
| source/server/auth/UserManager.ts | Add getUserById() and clarify doc for getUserByName(). |
| source/server/__test_fixtures/cube.obj | Add OBJ fixture for conversion/testing. |
| source/server/__test_fixtures/cube.mtl | Add MTL fixture for conversion/testing. |
| source/server/__test_fixtures/cube.glb | Add/refresh GLB fixture. |
| source/server/__test_fixtures/cube_webp.glb | Add/refresh GLB fixture with WEBP textures. |
| source/server/__test_fixtures/cube_textured.glb | Add textured GLB fixture. |
| source/server/__test_fixtures/cube_meshopt.glb | Add meshopt-compressed GLB fixture. |
| source/server/__test_fixtures/cube_etc1s.glb | Add ETC1S/KTX2 GLB fixture. |
| source/server/__test_fixtures/cube_draco.glb | Add draco-compressed GLB fixture. |
| source/e2e/tsconfig.json | New TS config for e2e package. |
| source/e2e/tests/userSettings.spec.ts | Update fixtures import and account creation pattern. |
| source/e2e/tests/upload_zip.spec.ts | Update upload UI interactions and created-scenes expectations. |
| source/e2e/tests/scene_settings.spec.ts | Update fixtures import path. |
| source/e2e/tests/download.spec.ts | Update expected archive paths for model location changes. |
| source/e2e/tests/accessRights.spec.ts | Update upload access expectations and fixtures usage. |
| source/e2e/playwright.config.ts | Use node:path import style. |
| source/e2e/package.json | Bump @types/node version. |
| source/e2e/package-lock.json | Lockfile updates for @types/node/undici-types. |
| source/e2e/fixtures.ts | Change uniqueAccount fixture into a factory with access level param. |
| source/e2e/__test_fixtures/Diffuse.jpg | Add image fixture for textured OBJ/MTL e2e. |
| source/e2e/__test_fixtures/cube.obj | Add OBJ fixture for e2e conversion flows. |
| source/e2e/__test_fixtures/cube.mtl | Add MTL fixture for e2e conversion flows. |
| Dockerfile | Add utilities stage to install KTX and runtime deps (OpenCL). |
| .github/workflows/build.yml | Drop Node 16 from CI matrix. |
Files not reviewed (1)
- source/e2e/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let srcMimeType = texture.getMimeType(); | ||
|
|
||
| if (options.slots && !slots.find((slot) => slot.match(options.slots))) { | ||
| logger.debug(`${prefix}: Skipping, [${slots.join(', ')}] excluded by "slots" parameter.`); | ||
| return; | ||
| }else if (srcMimeType === 'image/ktx2') { | ||
| logger.debug(`${prefix}: Skipping, already KTX.`); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Inside the texture loop, several branches use return to skip a texture (slots mismatch, already KTX, etc.). Because this is inside the transform callback, return exits the whole transform and prevents processing the remaining textures. Use continue (or restructure) so only the current texture is skipped.
source/server/utils/filetypes.ts
Outdated
| export async function readMagicBytes(filepath: string): Promise<string>{ | ||
| let handle = await fs.open(filepath, fs.constants.O_RDONLY); | ||
| try{ | ||
| const b = Buffer.allocUnsafe(12); |
There was a problem hiding this comment.
readMagicBytes() calls fs.constants.O_RDONLY but fs is imported from fs/promises, which doesn't expose constants in Node. This will throw at runtime. Import constants from node:fs (or use numeric flag 0) instead.
| const {filename, size: filesize} = task.data; | ||
| const contentRange = req.get("Content-Range"); | ||
|
|
||
| const contentLength = parseInt(req.get("Content-Length")!); | ||
| if(!contentLength || !Number.isInteger(contentLength)) throw new LengthRequiredError(`A valid Content-Length header must be provided`); | ||
|
|
||
| if(task.status !== "initializing"){ | ||
| throw new BadRequestError(`Task ${id} is in state ${task.status}, which does not allow further data to be sent`); | ||
| } | ||
|
|
||
|
|
||
| //Call this once the upload has completed | ||
| async function processUpload(){ | ||
| await taskScheduler.runTask({ task, immediate: true, handler: parseUserUpload as any }); | ||
| } | ||
|
|
||
| const filepath = path.join(vfs.getTaskWorkspace(task.task_id), filename); | ||
|
|
There was a problem hiding this comment.
filename comes from task data (user-controlled) and is joined into a filesystem path. A value like ../../foo will escape the task workspace and allow writing arbitrary files. Sanitize to a safe basename and/or verify the resolved path stays within vfs.getTaskWorkspace() before writing.
source/server/routes/scenes/post.ts
Outdated
| export function getFilename(headers: Request["headers"]) :string|undefined{ | ||
| const disposition = headers["content-disposition"]?contentDisposition.parse(headers["content-disposition"]): null; | ||
| if(disposition?.parameters?.filename) return disposition?.parameters?.filename; | ||
| const mimeType = headers["content-type"] ?? "application/octet-stream"; | ||
| const ext = extFromType(mimeType); | ||
| if(ext){ | ||
| return uid(12) + ext; | ||
| } |
There was a problem hiding this comment.
getFilename() returns Content-Disposition's filename verbatim (user-controlled). This value is later joined into a filesystem path, enabling path traversal (e.g. ../../...). Strip directory components (e.g. path.basename) and reject absolute/parent paths before using it.
source/server/routes/scenes/post.ts
Outdated
| handler: async function handlePostScene({task, context:{vfs, logger}}) :Promise<ImportSceneResult[]>{ | ||
| const dir = await vfs.createTaskWorkspace(task.task_id); | ||
| const abs_filepath = path.join(dir, filename); | ||
| const relPath = vfs.relative(abs_filepath); | ||
| logger.log("Write upload file to :", relPath); |
There was a problem hiding this comment.
filename (from headers) is joined into abs_filepath without validation. Even if getFilename() is fixed, this is the sink where path traversal can happen; ensure the resolved path remains within the task workspace before creating the write stream.
| xhr.onerror = function onUploadError(ev){ | ||
| console.log("XHR Error", ev); | ||
| setError({ code: xhr.status ||DOMException.NETWORK_ERR, message: xhr.response.message || xhr.statusText || navigator.onLine? "Server is unreachable": "Disconnected" }); | ||
| } |
There was a problem hiding this comment.
xhr.onerror builds the message with xhr.response.message || xhr.statusText || navigator.onLine ? ... which is parsed as (xhr.response.message || xhr.statusText || navigator.onLine) ? ... due to operator precedence. This will often return "Server is unreachable" even when disconnected. Also xhr.response is typically a string unless responseType is set, so .message will be undefined. Build the message explicitly with parentheses and use xhr.responseText (or set responseType='json').
source/ui/state/uploader.ts
Outdated
| if (299 < xhr.status) { | ||
| const fail_response = JSON.parse(xhr.responseText) as { message?: string }; | ||
| console.error("Upload Request failed :", fail_response.message ? fail_response.message : xhr.statusText) | ||
| setError({code: xhr.status, message: fail_response.message ? fail_response.message : xhr.statusText}); |
There was a problem hiding this comment.
In xhr.onload, JSON.parse(xhr.responseText) is done unconditionally for any HTTP error status. If the server returns non-JSON (or an empty body), this will throw and prevent the upload from being marked errored cleanly. Wrap parsing in try/catch and fall back to xhr.statusText/raw text.
| /** | ||
| * Close the queue gracefully | ||
| * Waits for active tasks to complete before aborting pending ones | ||
| * @param timeoutMs Maximum time to wait for active tasks. Defaults to 30 seconds. | ||
| */ | ||
| async close(timeoutMs: number = 1000){ | ||
| if(this.#c.signal.aborted){ |
There was a problem hiding this comment.
The JSDoc says close() defaults to a 30s timeout, but the implementation defaults to 1000ms. Either update the default to match the doc (30_000) or fix the docstring so shutdown behavior is clear and consistent.
| <h3 id="createdScenes-heading">{{i18n "titles.createdScenes" }}</h3> | ||
| <a href="." class="text-error" style="position: absolute; top:.5rem; right:.5rem; text-decoration: none;">🗙</a> | ||
|
|
There was a problem hiding this comment.
This close link is rendered as just an icon character (🗙) with no accessible name. Add an aria-label/title (or visually hidden text) so screen readers can announce what the control does.
| <h1> | ||
| Task #{{root.task_id}} — <span class="task-type">{{root.type}}</span> | ||
| <a href="." class="title-caret"> | ||
| ‹ | ||
| </a> |
There was a problem hiding this comment.
The back link in the <h1> uses only a single glyph (‹) as its content, which has no accessible name for screen readers. Add an aria-label/title (or include hidden text) describing the navigation action (e.g. "Back to tasks").
fa243ba to
efac7ba
Compare
Basic implementation of a task processing subsystem, capable of offloading heavy file processing to worker threads or external processes.
Initial implementation will be without explicit worker support but the code should be structured with that goal in mind.
OP#50