Conversation
Dependency ReviewThe following issues were found:
|
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
- Update package.json: rename to blacktrigram, add npm metadata, exports, peerDependencies, publishConfig - Add tsconfig.lib.json for TypeScript declaration generation - Add vite.lib.config.ts for Vite library mode bundling - Add .npmignore to control npm package contents - Add build:lib script for library build - Add publish job to release.yml workflow (following EP MCP Server pattern) - Update .gitignore to exclude lib/ build output - Update eslint.config.js to ignore lib/ directory - Update tsconfig.node.json to include vite.lib.config.ts Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/d41b4334-7914-41a8-85e3-7cabc868f149
…d fix npm pack verification Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/d41b4334-7914-41a8-85e3-7cabc868f149
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
…hing - Update typedoc.json sidebarLinks to match riksdagsmonitor pattern with all architecture docs (Architecture, Security Architecture, Threat Model, Data Model, Flowchart, State Diagram, Mindmap, SWOT, Future Architecture, Future Security) and ISMS references (Secure Dev Policy, Open Source Policy) - Update typedoc.json navigationLinks with npm Package and ISMS links - Update release-drafter.yml with npm package section, provenance verification, architecture documentation links, correct API docs URL (blacktrigram.com/api/), and additional resources section Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/91ac18bc-ace4-465e-84b2-b7f3c5bd47ef
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
There was a problem hiding this comment.
Pull request overview
This PR expands the project’s documentation/release metadata (TypeDoc + release-drafter) and also introduces npm library packaging + publishing automation for the blacktrigram package.
Changes:
- Update
typedoc.jsonnavigation/sidebar links to include architecture and ISMS-PUBLIC references. - Enhance release notes template with npm package installation/provenance and updated documentation URLs.
- Add npm library build/publish support (new Vite lib config, TS lib config, npm metadata/exports, npmignore/gitignore/eslint ignores, and a release workflow publish job).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.lib.config.ts | Adds Vite library-mode build configuration (ESM, preserveModules, externals). |
| typedoc.json | Aligns navigation/sidebar links with the desired documentation pattern (architecture + ISMS links). |
| tsconfig.node.json | Includes the new Vite library config in node TS project. |
| tsconfig.lib.json | Adds a dedicated TS config for generating declaration output into lib/. |
| package.json | Converts project metadata for npm publishing, adds exports/files/scripts, and peer deps. |
| eslint.config.js | Ignores lib/ output directory. |
| .npmignore | Defines publish-time exclusions (source, configs, tests, most docs). |
| .gitignore | Ignores lib/ build output. |
| .github/workflows/release.yml | Adds a new publish job that builds and publishes the npm package with provenance. |
| .github/release-drafter.yml | Adds npm package + architecture documentation sections; updates reporting URLs. |
| publish: | ||
| name: Publish npm Package | ||
| needs: [prepare, release] | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| id-token: write # Required for npm provenance |
There was a problem hiding this comment.
The PR metadata describes this as a docs-only change (TypeDoc + release-drafter), but this workflow hunk introduces a new publish job that publishes to npm. Please either update the PR title/description to reflect the npm-publishing scope, or split the npm publishing changes into a separate PR so reviewers can assess release/publishing risk independently.
package.json
Outdated
| "build:profile": "tsc -b && vite build --mode production --profile --minify esbuild", | ||
| "build:stats": "tsc -b && vite build --mode production && npx vite-bundle-analyzer dist/assets/*.js", | ||
| "build:fast": "tsc -b --incremental && vite build --mode production --minify esbuild", | ||
| "build:lib": "rm -rf lib && tsc -p tsconfig.lib.json && vite build --config vite.lib.config.ts && rm -rf lib/assets", |
There was a problem hiding this comment.
build:lib deletes lib/assets after the Vite library build. The library exports App (via src/index.ts), and App (and several components) import CSS, which Vite/Rollup typically emits as assets. Deleting lib/assets can leave JS files with missing CSS/asset imports, breaking consumers at build time. Remove the rm -rf lib/assets step or adjust the library build so CSS/assets are either emitted to a preserved location or not referenced from the emitted JS.
| "build:lib": "rm -rf lib && tsc -p tsconfig.lib.json && vite build --config vite.lib.config.ts && rm -rf lib/assets", | |
| "build:lib": "rm -rf lib && tsc -p tsconfig.lib.json && vite build --config vite.lib.config.ts", |
package.json
Outdated
| "@react-three/fiber": "^9.0.0", | ||
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", | ||
| "three": "^0.170.0" |
There was a problem hiding this comment.
peerDependencies.three is set to ^0.170.0, which does not include the Three.js version this repo builds/tests against (dependencies.three is 0.183.2). This will make npm/yarn/pnpm report unsatisfied peer deps for the common case and can block installs. Align the peer range with the supported Three.js version(s) (e.g., ^0.183.0 if you intend to support the current minor line).
| "three": "^0.170.0" | |
| "three": "^0.183.0" |
package.json
Outdated
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", | ||
| "three": "^0.170.0" | ||
| }, |
There was a problem hiding this comment.
This package now lists React/Three/R3F/Drei in both dependencies and peerDependencies. For a library, shipping React (and often Three/R3F) as direct dependencies can cause duplicate installations and runtime issues (e.g., invalid hook calls from multiple React copies). Prefer keeping these as peerDependencies (and devDependencies for local build/tests) unless you explicitly want to bundle them for consumers.
package.json
Outdated
| "SECURITY.md", | ||
| "CHANGELOG.md" |
There was a problem hiding this comment.
files includes CHANGELOG.md, but there is no CHANGELOG.md in the repository. This is misleading and will not include the intended changelog in the published tarball. Either add the file (even if generated) or remove it from files/.npmignore allowlist.
| "SECURITY.md", | |
| "CHANGELOG.md" | |
| "SECURITY.md" |
|
@copilot apply changes based on the comments in this thread |
- Remove rm -rf lib/assets from build:lib to preserve CSS/assets for consumers - Move React/Three.js/R3F/Drei from dependencies to devDependencies (keep as peerDependencies for library consumers) - Fix peerDependencies.three from ^0.170.0 to ^0.183.0 to match actual version - Remove non-existent CHANGELOG.md from files array Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/e51be5d8-c054-4b7e-94fb-63212a02eae3
Applied all 5 review comments in commit
|
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/b18c2fb5-f387-4de6-aec0-3ca2bcd27d95
Fixed the asset audit failure in The unit test failure ( |
…lse positive Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/b18c2fb5-f387-4de6-aec0-3ca2bcd27d95
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
src/utils/assetConfig.ts
Outdated
| if (!assetBasePath) { | ||
| return path; | ||
| } | ||
| // Ensure path starts with / for correct concatenation | ||
| const normalizedPath = path.startsWith("/") ? path : `/${path}`; |
There was a problem hiding this comment.
resolveAssetPath normalizes missing leading slashes only when assetBasePath is set. With the current early return, resolveAssetPath("assets/...") returns a relative URL when assetBasePath === "", which defeats the normalization behavior and can produce incorrect asset URLs. Consider normalizing the input path before the !assetBasePath check (or explicitly rejecting non-root-relative paths).
| if (!assetBasePath) { | |
| return path; | |
| } | |
| // Ensure path starts with / for correct concatenation | |
| const normalizedPath = path.startsWith("/") ? path : `/${path}`; | |
| // Ensure path starts with / for correct concatenation | |
| const normalizedPath = path.startsWith("/") ? path : `/${path}`; | |
| if (!assetBasePath) { | |
| return normalizedPath; | |
| } |
| * Resolve an asset path by prepending the configured base path. | ||
| * | ||
| * @param path - Root-relative asset path starting with a forward slash, e.g. /assets/audio/music/intro_theme.mp3 | ||
| * @returns The resolved URL with the configured base path prepended |
There was a problem hiding this comment.
The JSDoc for resolveAssetPath says the path parameter must be root-relative and start with /, but the implementation/tests also support inputs without a leading slash. Please update the @param description (and the top-level module comment if needed) to document the accepted formats consistently.
| describe("resolveAssetPath", () => { | ||
| it("returns path unchanged when no base path set", () => { | ||
| expect(resolveAssetPath("/assets/audio/music/intro_theme.mp3")).toBe( | ||
| "/assets/audio/music/intro_theme.mp3", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
There is no unit test covering the case where no base path is set and the input asset path is missing a leading slash (e.g. resolveAssetPath("assets/...")). Adding this test will lock in the expected normalization behavior and prevent regressions.
|
@copilot apply changes based on the comments in this thread |
…add missing-slash test Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/1ac44a25-578b-473e-bb56-9be7afc3df34
Applied all 3 review comments in
|
.github/workflows/release.yml
Outdated
| - name: Cache dependencies | ||
| uses: actions/cache@668228422ae6a00e4ad889ee87cd7109ec5666a7 # v5.0.4 | ||
| with: | ||
| path: ~/.npm | ||
| key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-node- | ||
|
|
There was a problem hiding this comment.
The publish job uses actions/setup-node with cache: "npm" and then also runs a separate actions/cache step for ~/.npm. These overlap (double-caching the same directory) and can add unnecessary CI time/complexity; consider keeping only one caching mechanism (either setup-node’s built-in npm cache or the explicit actions/cache step).
| - name: Cache dependencies | |
| uses: actions/cache@668228422ae6a00e4ad889ee87cd7109ec5666a7 # v5.0.4 | |
| with: | |
| path: ~/.npm | |
| key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} | |
| restore-keys: | | |
| ${{ runner.os }}-node- |
📸 Automated UI Screenshots📋 Screenshots Captured (8)
📦 Download Screenshots📥 Download all screenshots from workflow artifacts
🤖 Generated by Playwright automation |
|
@copilot apply changes based on the comments in this thread |
…dy caches) Co-authored-by: pethers <1726836+pethers@users.noreply.github.com> Agent-Logs-Url: https://github.com/Hack23/blacktrigram/sessions/2ade8cfc-d557-4081-8d63-c77ab822a848
Removed the duplicate |
v-prefix, add injectablesetAssetBasePath/resolveAssetPathAPI for library consumersCollisionDetection.test.tsperformance test is pre-existing flaky (timing-sensitive, not caused by this PR)💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.