ENG-757 experiment with pnpm #352
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
4 times, most recently
from
d2d3641 to
7a9c1d9
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
some thoughts/questions
apps/roam/tsconfig.json
Outdated
| "noUncheckedIndexedAccess": false | ||
| "noUncheckedIndexedAccess": false, | ||
| "paths": { | ||
| "@repo/*": ["../../packages/*"], |
There was a problem hiding this comment.
What problem was this solving?
There was a problem hiding this comment.
The linter gets confused when we use "@repo/" notation, at least on zed.
If you want to test on vscode, have a look at src/utils/conceptConversion.ts for example, the import at line 6 is red for me without this line.
There was a problem hiding this comment.
Yeah, this seems like a deeply rooted problem ...
There was a problem hiding this comment.
This this fix feels like a Band-Aid that will bite us later.
There was a problem hiding this comment.
I tested this and could not repo the issue. "@repo/*": ["../../packages/*"], is not required for me. But neither is "~/*": ["./src/*"] required so I think this is actually reflecting a different problem.
|
|
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
6e03010 to
2564255
Compare
|
|
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
2564255 to
c8243bd
Compare
|
Note: To use the pnpm patching mechanism, I had to use the latest pnpm@10. Vercel wants an environment variable for that: |
|
ah damn got into a circular dependency. The real fix will take some time... |
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
2 times, most recently
from
e5b07e4 to
83d9e6a
Compare
maparent
changed the base branch from
main
to
eng-633-refactor-move-some-functions-to-packagesdatabase
|
ok, rewritten over 633. |
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
83d9e6a to
548a07e
Compare
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
b2b6749 to
bd2a796
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughMigrates the repository from npm to pnpm across CI workflows, scripts, and documentation. Introduces pnpm workspace configuration with catalogs and patches. Updates multiple package.json files, TypeScript path mappings, and build/deploy scripts, including Roam deploy overwrite behavior and Obsidian publishing/build commands. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
patches/@tldraw__editor@2.3.0.patch (1)
1-24: DefaultErrorFallback: ensureuseEffectis imported before useYou added a
useEffectcall to dispatch a custom event. Make sureuseEffectis imported from react in this module; otherwise this will throw at runtime.Suggested patch hunk header (add if missing at the import section of DefaultErrorFallback.mjs):
-// (imports above) +import { useEffect } from 'react';Optional: consider guarding against SSR (no
document) if this file can be imported server-side.- document.dispatchEvent(event); + if (typeof document !== 'undefined') { + document.dispatchEvent(event); + }patches/tldraw@2.3.0.patch (1)
61-81: React 17 fallback: avoid fixed IDs; generate a stable unique idReplacing
useSafeId()with a constant string can create duplicate IDs when multiple toolbars are rendered, breaking a11y and event associations. You already imported nanoid—use it.Apply:
-import { nanoid } from "nanoid"; +import { nanoid } from "nanoid"; +import { useMemo } from "react"; - const id = "overflowing-toolbar"; + const id = useMemo(() => `overflowing-toolbar-${nanoid(6)}`, []);If you prefer not to add
useMemo, capturing the ID in a ref also works:+import { useRef } from "react"; +const idRef = useRef(`overflowing-toolbar-${nanoid(6)}`); +const id = idRef.current;packages/ui/package.json (1)
37-48: Move React, React-DOM, Tailwind and SHADCN into peer/devDependenciesTo prevent bundled React and mismatched Tailwind across consuming apps, the UI package should only depend on runtime styling libraries and component utilities.
- Remove
react,react-domandtailwindcssfrom dependencies.- Add them as peerDependencies so that the host app provides them:
•"react": "^18 || ^19"
•"react-dom": "^18 || ^19"
•"tailwindcss": "^3.4.0"- Keep those same versions in devDependencies for build/types:
•"react": "^19.1"
•"react-dom": "^19.1"
•"tailwindcss": "^3.4.17"- Move the
shadcnCLI to devDependencies (or call via npx) rather than bundling it at runtime.- Retain only pure runtime UI deps under
dependencies:
•class-variance-authority,clsx,lucide-react,tailwind-merge,tailwindcss-animate,tw-animate-css.Diff suggestion for packages/ui/package.json:
"dependencies": { - "class-variance-authority": "^0.7.1", - "clsx": "^2.1.1", - "lucide-react": "^0.468.0", - "react": "^19", - "react-dom": "^19", - "shadcn": "2.10.0", - "tailwindcss": "^3.1.0", - "tailwind-merge": "^3.3.1", - "tailwindcss-animate": "^1.0.7", - "tw-animate-css": "^1.3.4" + "class-variance-authority": "^0.7.1", + "clsx": "^2.1.1", + "lucide-react": "^0.468.0", + "tailwind-merge": "^3.3.1", + "tailwindcss-animate": "^1.0.7", + "tw-animate-css": "^1.3.4" }, + "peerDependencies": { + "react": "^18 || ^19", + "react-dom": "^18 || ^19", + "tailwindcss": "^3.4.0" + }, + "devDependencies": { + // existing devDependencies… + "react": "^19.1", + "react-dom": "^19.1", + "tailwindcss": "^3.4.17", + "shadcn": "2.10.0" + }
♻️ Duplicate comments (2)
apps/roam/tsconfig.json (2)
12-12: rootDir set to "../.." is a broad scope; consider project references insteadThis unblocks TS’s “ambiguous root” errors but widens the compilation context. Longer-term, consider TS project references and package exports to resolve @repo/* cleanly without widening rootDir.
If you’d like, I can sketch a minimal tsconfig.base + references setup to replace the Band-Aid.
19-21: Note on @repo/ path alias*Pointing directly to ../../packages/* imports source rather than package entrypoints. That can bypass package boundaries and type isolation. If feasible, prefer consuming the package via its name and exports (pnpm workspace protocol) and let TS resolve through references.
🧹 Nitpick comments (35)
README.md (1)
47-49: Prefer Corepack over global pnpm; align local commands with PNPM usageTo keep developer environments consistent with the repo’s pinned packageManager and CI, consider Corepack instead of a global npm install, and invoke Turbo via PNPM so the workspace-local binary is used.
Apply this diff to the install step:
- npm install -g pnpm@10 - pnpm install + corepack enable + # keep this in sync with the root package.json "packageManager" field + corepack prepare pnpm@10.15.0 --activate + pnpm installOptionally, prefer calling Turbo through PNPM (helps on machines where
turboisn’t globally on PATH):- turbo dev + pnpm exec turbo devNotes:
- Pinning pnpm to 10.15.0 matches the repo’s packageManager field and avoids silent minor upgrades.
- This also removes the need for global installs and aligns with how your CI sets up PNPM.
Also applies to: 54-55
apps/roam/build.sh (1)
1-6: Harden the build script: freeze lockfile, use pnpm exec for Turbo, and copy dist robustlyThe current script will work, but a few small tweaks improve reproducibility and resilience:
- Use
--frozen-lockfileto guarantee builds use the committed lockfile.- Prefer
pnpm exec turbo …instead ofnpxto avoid pulling Turbo via npm.- Copy the whole
distdirectory structure (avoids failures when there are nested files or when shell globs don’t match).#!/bin/bash -set -e -pnpm install -npx turbo run build --filter=roam -cp dist/* . +set -euo pipefail +pnpm install --frozen-lockfile +pnpm exec turbo run build --filter=roam +cp -a dist/. .If this script is invoked outside the repo root, consider resolving and operating from the workspace root explicitly:
repo_root="$(git rev-parse --show-toplevel)" cd "$repo_root"apps/obsidian/scripts/publish.ts (1)
258-258: Optional: add package-manager fallback and version guardOn machines where PNPM isn’t on PATH (e.g., fresh Node installations with Corepack not activated), you can auto-fallback to npm and/or assert PNPM ≥ 10 for consistency with patches.
Apply this diff locally to use a resolved package manager:
- await execCommand("pnpm run build", { cwd: dir }); + const pm = await resolvePackageManager(); + await execCommand(`${pm} run build`, { cwd: dir });Add this helper somewhere above:
async function resolvePackageManager(preferred = "pnpm"): Promise<"pnpm" | "npm"> { try { await execPromise(`${preferred} -v`); return preferred; } catch { return "npm"; } }If you want to hard-require PNPM 10+, insert a version check:
async function assertPnpmVersion(min = [10, 0, 0]) { const out = await execPromise("pnpm -v").then(r => r.stdout.trim()); const cur = out.split(".").map(n => parseInt(n, 10)); for (let i = 0; i < 3; i++) { if ((cur[i] ?? 0) > (min[i] ?? 0)) return; if ((cur[i] ?? 0) < (min[i] ?? 0)) throw new Error(`pnpm ${min.join(".")}+ required, found ${out}`); } }.github/workflows/ci.yaml (1)
25-25: Optional: tie the cache to the lockfile path for more stable hits.This helps avoid stale cache scenarios if you ever add secondary lockfiles or restructure the repo.
Apply this diff:
- name: Setup Node.js environment uses: actions/setup-node@v4 with: node-version: 20 - cache: "pnpm" + cache: "pnpm" + cache-dependency-path: pnpm-lock.yaml.github/workflows/roam-pr.yaml (1)
41-41: Use pnpm to run Turbo for consistency and cachingpnpm will invoke the locally-installed Turbo (v^2.5.4) from your workspace and leverage pnpm’s store/cache, avoiding pulls via npm and ensuring the pinned version is used.
• File: .github/workflows/roam-pr.yaml
• Line: 41- - name: Deploy - run: npx turbo run deploy --filter=roam + - name: Deploy + run: pnpm turbo run deploy --filter=roamapps/roam/scripts/deploy.ts (2)
72-77: Prevent stale CDN/browser caches on overwrite.Overwriting the same public URL can serve stale content unless you set cache headers. Also consider setting contentType to ensure correct MIME types.
Apply this diff to add a conservative Cache-Control policy (tweak as desired):
const blob = await put(pathname, content, { access: "public", addRandomSuffix: false, allowOverwrite: true, + cacheControl: "public, max-age=0, must-revalidate", + // contentType: guessContentType(file), token, });Add this small helper near the top (outside the selected range) if you want explicit content types:
const guessContentType = (filename: string): string | undefined => { if (filename.endsWith(".js")) return "text/javascript; charset=utf-8"; if (filename.endsWith(".css")) return "text/css; charset=utf-8"; if (filename.endsWith(".json")) return "application/json; charset=utf-8"; if (filename.endsWith(".md")) return "text/markdown; charset=utf-8"; return undefined; // let the API infer if not matched };
83-84: Fix link output for main vs branch paths.Uploads to main omit the branch segment, but the printed URL always includes it.
- const url = `https://discoursegraphs.com/releases/${resolvedWorkspace}/${resolvedBranch}`; - console.log(`🔗 ${url}`); + const url = + resolvedBranch === "main" + ? `https://discoursegraphs.com/releases/${resolvedWorkspace}` + : `https://discoursegraphs.com/releases/${resolvedWorkspace}/${resolvedBranch}`; + console.log(`🔗 ${url}`);apps/website/tsconfig.json (1)
5-5: Avoid setting rootDir to the monorepo root.Next.js doesn’t emit with tsc, so rootDir mainly influences diagnostics and can accidentally widen the project boundary. Keep it local (or omit).
- "rootDir": "../..", + // "rootDir": ".", // or remove entirely.github/workflows/database-deploy.yaml (3)
36-36: Use pnpm to invoke turbo for consistency and cache reuseSince the workspace is PNPM-based and dependencies are installed, prefer pnpm turbo over npx turbo to ensure it uses the locally installed version and PNPM’s store.
- - run: npx turbo deploy -F @repo/database + - run: pnpm turbo deploy -F @repo/database
32-35: Pin Supabase CLI version (avoid latest in CI)Using latest risks breaking builds on upstream releases. Pin a known-good version or provide an overridable env.
- - uses: supabase/setup-cli@v1 - with: - version: latest + - uses: supabase/setup-cli@v1 + with: + # TODO: set to a tested version and keep in sync repo-wide + version: "2.XX.X"If you prefer configurability, add an env SUPABASE_CLI_VERSION at the job level and reference it here.
18-31: Enforce strict lockfile and confirm PNPM version alignmentVerified that the declared packageManager (
pnpm@10.15.0) matches the CI setup (pnpm v10.15.0), so no changes are needed there. To ensure CI fails on any lockfile drift, update the install step to use the stricter--frozen-lockfileflag.• File: .github/workflows/database-deploy.yaml
• Step: Install Dependencies- - name: Install Dependencies - run: pnpm install --prefer-frozen-lockfile + - name: Install Dependencies + run: pnpm install --frozen-lockfile.github/workflows/roam-main.yaml (3)
31-36: CI installs: use --frozen-lockfile for deterministic buildsPrefer strict lockfile in CI; --prefer-frozen-lockfile can silently update and commit drift later.
- - name: Install Dependencies - run: pnpm install --prefer-frozen-lockfile + - name: Install Dependencies + run: pnpm install --frozen-lockfile
37-38: Invoke turbo via pnpm for toolchain consistencyUse pnpm turbo rather than npx turbo.
- - name: Deploy - run: npx turbo run deploy --filter=roam + - name: Deploy + run: pnpm turbo run deploy --filter=roam
15-17: Optional: add concurrency to auto-cancel superseded runs on push to mainPrevents piling up deploys when multiple commits land quickly.
jobs: deploy: + concurrency: + group: roam-main-${{ github.ref }} + cancel-in-progress: true runs-on: ubuntu-latestapps/roam/tsconfig.json (2)
17-18: Confirm JSX runtime setting"jsx": "react" uses the classic runtime. If the app uses the automatic runtime (React 17+), switch to "react-jsx" to simplify imports and align with modern toolchains.
- "jsx": "react", + "jsx": "react-jsx",
3-9: Consider excluding build artifacts/tests from emitYou include tests and scripts; if this project emits JS/DTs, exclude tests or set noEmit for this app, depending on your build pipeline.
"include": [ "src", "src/types.d.ts", "tailwind.config.ts", "tests", "scripts" ], + "exclude": ["dist", "**/*.test.*", "**/__tests__/**"],packages/database/supabase/functions/create-space/deno.json (1)
2-11: Optional: pin jsr versions and consider importing built entrypoints
- jsr:@supabase/*@2 without a patch pin may drift.
- If utils provides a built ESM entrypoint, consider targeting that (to avoid coupling to src paths). Deno can import TS, so this is optional.
.github/workflows/roam-release.yaml (2)
32-34: Lockfile strictness and turbo invocationMirror the other workflows: use --frozen-lockfile and pnpm turbo.
- - name: Install Dependencies - run: pnpm install --prefer-frozen-lockfile + - name: Install Dependencies + run: pnpm install --frozen-lockfile- - name: Update Roam Depot Extension - run: npx turbo run publish --filter=roam + - name: Update Roam Depot Extension + run: pnpm turbo run publish --filter=roam
10-16: Optional hardening: restrict permissions to least privilegeYou already request write perms; consider limiting the GITHUB_TOKEN’s default permissions at the workflow level and granting only what’s needed per job/step.
packages/database/README.md (1)
31-31: Docs: pnpm command switches look good; fix a typo and clarify script invocation
- The pnpm replacements are consistent and correct.
- Typo: "another other terminal" on Line 65.
- Minor clarity: with pnpm, arguments after -- are forwarded to the script; worth stating explicitly next to genenv.
Apply:
-2. In another other terminal, `cd` to this directory (`packages/database`) and run the tests with `pnpm run test` +2. In another terminal, `cd` to this directory (`packages/database`) and run the tests with `pnpm run test`Optional clarification:
-You would run `pnpm run genenv -- branch` in this directory; +You would run `pnpm run genenv -- branch` in this directory (note: everything after `--` is passed to the script by pnpm);Also applies to: 35-35, 65-65, 77-77
patches/@tldraw__editor@2.3.0.patch (1)
29-75: React 17 compatibility: remove unusedflushSyncand simplify render lifecycleSwitch to ReactDOM.render is correct for React 17. Two follow-ups:
flushSyncis no longer used; remove the import to avoid dead code.- Rendering into a detached fragment works, but you don't need a DocumentFragment here.
Apply:
-import { flushSync } from "react-dom"; -// import { createRoot } from "react-dom/client"; +// import { createRoot } from "react-dom/client"; +// flushSync not needed when using React 17's ReactDOM.render +// import { flushSync } from "react-dom"; async getSvgElement(shapes, opts = {}) { const result = await getSvgJsx(this, shapes, opts); - if (!result) return undefined; - - const fragment = document.createDocumentFragment(); - const container = document.createElement('div'); - fragment.appendChild(container); - - ReactDOM.render(result.jsx, container); - - const svg = container.firstElementChild; - assert(svg instanceof SVGSVGElement, 'Expected an SVG element'); - - ReactDOM.unmountComponentAtNode(container); + if (!result) return undefined; + + const container = document.createElement('div'); + ReactDOM.render(result.jsx, container); + const svg = container.firstElementChild; + assert(svg instanceof SVGSVGElement, 'Expected an SVG element'); + ReactDOM.unmountComponentAtNode(container); return { svg, width: result.width, height: result.height }; }patches/tldraw@2.3.0.patch (1)
96-117: Style merge order is correct; maintainability nitSpreading
...stylelast preserves consumer overrides—good. Optional: co-locate prop type updates if there is a.d.tsforTldrawUiIcon.No code changes required; just ensure types are in sync.
pnpm-workspace.yaml (1)
47-51: Patched dependencies: solid; keep patches synced with upstream versionsPatch mapping looks correct. Since
allowUnusedPatches: falseis set, upgrades of tldraw/@tldraw/editor will hard-fail if patches drift—which is good. Consider adding a short maintainer note in the README about how to regenerate these patches when bumping versions.No code changes required here.
apps/obsidian/scripts/README.md (5)
12-12: Prefer Corepack over global pnpm installRecommend documenting Corepack to pin pnpm from the repo’s packageManager instead of requiring a global install. This avoids version drift across environments.
-1. **Node.js and pnpm** - Ensure you have Node.js 20+ installed, with pnpm@10 installed globally (`npm install -g pnpm`) +1. **Node.js and pnpm** - Ensure you have Node.js 20+. Enable Corepack so the repo-pinned pnpm is used: + ```bash + corepack enable + corepack prepare pnpm@$(node -p "require('./package.json').packageManager.split('@')[1]") --activate + ```
148-148: Fix heading trailing punctuationRemove the trailing colon to satisfy MD026 and keep headings consistent.
-### For All Releases: +### For All Releases
187-189: Minor phrasing tweak for consistencyConsider “External pre-release” vs “External Pre-release” capitalization consistency; the table uses “External Pre-release”. Not critical, but consistency helps.
199-201: Troubleshooting snippet: consider showing pnpm variantSince the repo is pnpm-first, it may help to include a pnpm example alongside tsx.
-tsx scripts/publish.ts --version 1.0.0 +# using tsx directly +tsx scripts/publish.ts --version 1.0.0 +# or via pnpm script +pnpm run publish -- --version 1.0.0
251-257: Fix heading trailing punctuation and tighten wordingRemove the trailing colon; also the bullet reads fine but can be slightly crisper.
-### Security: +### Security - Uses GitHub API instead of git commands for better security -- Never commits tokens to repository +- Never commits tokens to the repositorypackages/database/package.json (1)
20-21: Order of build steps: consider generating env before buildIf
scripts/build.tsneeds env vars, rungenenvfirst. If not, current order is fine—just confirming intent.-"build": "tsx scripts/build.ts && pnpm run genenv -- local", +"build": "pnpm run genenv -- local && tsx scripts/build.ts",apps/website/package.json (1)
20-38: Version alignment with UI package peersEnsure the UI package exposes
reactandreact-domas peerDependencies (see UI comment). If so, the app’s^19.1is fine. If UI keeps them as regular dependencies, you risk duplicate Reacts at runtime.apps/roam/package.json (5)
19-21: Align React + @types with BlueprintJS 3 and tldraw to avoid peer-dep conflictsBlueprintJS 3 typically expects older React majors, while tldraw 2.x and some chart libs tend to require React 18+. With pnpm’s strict peer resolution, a mismatch will fail installs or produce hard warnings. Action items:
- Confirm what versions “catalog:roam” resolves for react/react-dom/@types/*.
- Ensure these satisfy all peer ranges for: @blueprintjs/core@3.x, react-draggable, react-vertical-timeline-component, roamjs-components, react-charts, and tldraw packages.
- If conflicts arise, either upgrade BlueprintJS (breaking UI change risk), or choose a tldraw version compatible with the chosen React, or isolate/reactify the tldraw surface into a separate build that bundles its own React.
Also applies to: 58-58, 60-60, 66-66
40-41: Supabase SDKs: double-check browser bundling and polyfillssupabase-js targets browsers but can drag in fetch/polyfills depending on usage. Please ensure esbuild config remains “platform=browser” and avoid node core polyfills. If any node shims are getting pulled, consider aliasing unwanted modules to false.
42-45: Avoid mixing “tldraw” umbrella with modular @tldraw/ packages*Having both tldraw and @tldraw/{editor,utils,validate} tends to duplicate code and increase bundle size; it can also cause subtle context/versioning issues under pnpm. Prefer one approach—ideally the modular packages.
Apply this diff to keep the modular set:
"@tldraw/editor": "2.3.0", "@tldraw/utils": "2.3.0", "@tldraw/validate": "2.3.0", @@ - "tldraw": "2.3.0",If you prefer the umbrella package, remove the three modular entries instead.
Also applies to: 65-65
49-49: Reassess core-js dependencyUnless you explicitly import targeted polyfills, core-js as a top-level dep can bloat the bundle. Consider removing it or fencing polyfills to only what’s needed (and only in the scripts/bundle that need them).
62-64: react-in-viewport is alpha; consider a maintained alternativeIf this is new usage, consider react-intersection-observer or a thin wrapper around IntersectionObserver to reduce risk from unmaintained alpha code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.github/workflows/ci.yaml(1 hunks).github/workflows/database-deploy.yaml(1 hunks).github/workflows/roam-main.yaml(2 hunks).github/workflows/roam-pr.yaml(1 hunks).github/workflows/roam-release.yaml(1 hunks)README.md(1 hunks)apps/obsidian/package.json(2 hunks)apps/obsidian/scripts/README.md(9 hunks)apps/obsidian/scripts/publish.ts(2 hunks)apps/obsidian/tsconfig.json(1 hunks)apps/roam/build.sh(1 hunks)apps/roam/package.json(2 hunks)apps/roam/scripts/deploy.ts(1 hunks)apps/roam/tsconfig.json(1 hunks)apps/website/package.json(1 hunks)apps/website/tsconfig.json(2 hunks)package.json(1 hunks)packages/database/README.md(3 hunks)packages/database/cucumber.json(1 hunks)packages/database/package.json(2 hunks)packages/database/supabase/functions/create-space/deno.json(1 hunks)packages/tailwind-config/package.json(1 hunks)packages/ui/package.json(2 hunks)patches/@tldraw__editor@2.3.0.patch(2 hunks)patches/tldraw@2.3.0.patch(6 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Prettier with the project's configuration
Applied to files:
apps/obsidian/tsconfig.jsonapps/roam/tsconfig.jsonapps/website/tsconfig.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Applied to files:
apps/roam/tsconfig.jsonpackage.jsonapps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/tsconfig.jsonapps/roam/build.shapps/roam/package.json
📚 Learning: 2025-07-19T22:34:16.794Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-07-19T22:34:16.794Z
Learning: Applies to apps/obsidian/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Obsidian plugin
Applied to files:
apps/obsidian/scripts/publish.tspnpm-workspace.yamlapps/obsidian/package.json
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to apps/website/**/*.{tsx,jsx,ts,js} : Use Tailwind CSS for styling where possible
Applied to files:
packages/ui/package.jsonpackages/tailwind-config/package.jsonapps/website/tsconfig.jsonapps/roam/package.json
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/README.md
📚 Learning: 2025-05-20T03:04:21.602Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/migrations/20250512142307_sync_table.sql:20-21
Timestamp: 2025-05-20T03:04:21.602Z
Learning: Database migration files (like the SQL files in packages/database/supabase/migrations/) should not be modified after they're created, even to fix minor issues like unused variables. Issues should be addressed in newer schema files instead to maintain migration history integrity.
Applied to files:
packages/database/README.md
📚 Learning: 2025-05-22T23:50:23.771Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/migrations/20250513173724_content_concept_key.sql:37-60
Timestamp: 2025-05-22T23:50:23.771Z
Learning: For the discourse-graph project, database schema management focuses on the final state in the supabase/schemas directory, not on the individual migration files. When reviewing database changes, consider only the schema definitions in this directory, not potential duplications or conflicts across migration files.
Applied to files:
packages/database/README.md
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/README.md
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/README.md
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/README.mdapps/website/tsconfig.jsonpackages/database/package.jsonapps/website/package.jsonapps/roam/package.json
📚 Learning: 2025-07-22T01:50:20.253Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-07-22T01:50:20.253Z
Learning: In packages/database/scripts/create_env.ts, the Vercel CLI integration uses both vercel/sdk for deployment queries and vercel CLI for environment variable pulling, with support for different variants (local, branch, production, all) and proper team/project configuration with discourse-graphs defaults.
Applied to files:
packages/database/README.mdpackages/database/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Roam Research extension
Applied to files:
apps/roam/build.shpackage.jsonapps/roam/package.json
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
package.jsonapps/roam/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.
Applied to files:
package.json
🪛 LanguageTool
apps/obsidian/scripts/README.md
[grammar] ~45-~45: There might be a mistake here.
Context: ...: alpha-* (comes first alphabetically) - External: beta-* (comes after alpha,...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...ion with alpha or beta suffixes - GitHub: Marked as "Pre-release" - **Mai...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...t the Script Does ### For All Releases: 1. Validates input - Checks version forma...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...ry unchanged, GitHub pre-release created External Stable: Repository main bra...
(QB_NEW_EN)
[grammar] ~254-~254: There might be a mistake here.
Context: ... version in manifest.json ### Security: - Uses GitHub API instead of git commands ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
apps/obsidian/scripts/README.md
148-148: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
254-254: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (25)
packages/database/cucumber.json (1)
3-3: Verify Cucumber loader invocation in packages/database
- ✅ ts-node-maintained is declared in packages/database/package.json (version ^10.9.5).
- ❓ I didn’t find any npm script in packages/database/package.json (or elsewhere) invoking
cucumber/cucumber-js. Please confirm that whichever command runs Cucumber (npm script, npx, CI job, etc.) launches Node with the ESM loader:
• via--loader ts-node-maintained/esm, or
• by picking up theloaderarray in packages/database/cucumber.json- 🛠️ If you don’t already have a script, consider adding to packages/database/package.json:
so that"scripts": { "cucumber": "cucumber-js --config cucumber.json" }npm run cucumberwill load ts-node-maintained/esm automatically.- 🔄 Alternatively, for a CommonJS setup you can remove the ESM loader and use:
to avoid ESM resolution concerns."requireModule": ["ts-node/register/transpile-only"]Please double-check your Cucumber invocation to ensure the loader is applied correctly under your PNPM workspace.
apps/obsidian/scripts/publish.ts (1)
258-258: LGTM: build now uses PNPMSwitching to
pnpm run buildaligns this script with the repo-wide PNPM migration. Looks good..github/workflows/ci.yaml (1)
28-28: Use--frozen-lockfilefor reproducible CI installsFor CI, prefer failing fast when the lockfile is out of sync to guarantee deterministic builds.
• File:
.github/workflows/ci.yamlLine: 28
Change the weaker flag to the strict one.Apply this diff:
- run: pnpm install --prefer-frozen-lockfile + run: pnpm install --frozen-lockfileUpdate your verification script to correctly search hidden workflow files and ensure only one lockfile exists at the repo root:
#!/usr/bin/env bash set -euo pipefail echo "Checking for any workflows using --prefer-frozen-lockfile:" rg -n --hidden --glob '.github/workflows/**/*.y*ml' 'prefer-frozen-lockfile' .github/workflows || echo "None found" echo "Verifying pnpm-lock.yaml presence (expect exactly 1 at repo root):" lockfiles=$(find . -maxdepth 2 -type f -name pnpm-lock.yaml) echo "$lockfiles" count=$(echo "$lockfiles" | wc -l) if [ "$count" -ne 1 ] || [ "$lockfiles" != "./pnpm-lock.yaml" ]; then echo "❌ Unexpected lockfiles detected!" exit 1 else echo "✅ Single pnpm-lock.yaml at repo root." fiapps/roam/scripts/deploy.ts (1)
75-75: LGTM: allowOverwrite=true is appropriate for stable release URLs.This matches the intent to publish to a fixed pathname per branch without random suffixes.
apps/website/tsconfig.json (1)
7-9: LGTM: @repo/ path mapping aligns with the PNPM workspace layout.*This should make cross-package imports explicit and type-safe.
.github/workflows/roam-main.yaml (1)
22-27: PNPM setup: looks goodInstalling pnpm via pnpm/action-setup@v4 before Node setup is fine and consistent with other workflows.
packages/database/supabase/functions/create-space/deno.json (1)
9-9: Alias switch to@repo/utils/lib/execContextis correctly applied
- No remaining references to the old alias (
@repo/ui/lib/execContext) underpackages/database/supabase/functions.- The only Supabase function importing
execContextis
packages/database/supabase/functions/create-space/deno.json, and it now points to@repo/utils/lib/execContext.- No other functions reference
execContext, so no further updates are necessary.✅ Approving these changes.
.github/workflows/roam-release.yaml (1)
20-25: PNPM setup step: goodThe action and version pin mirror other workflows. Consistency helps CI stability.
package.json (1)
21-21: CI pnpm version check neededIt appears no references to
pnpm@10.15.0were detected in your GitHub workflow files. Please manually verify that:
- Every workflow in
.github/workflows/*.ymlor.github/workflows/*.yamlpins pnpm to version 10.15.0, for example via:
uses: pnpm/action-setup@10.15.0- or setting
with: version: 10.15.0- Any direct
pnpmcommands in scripts or composite actions rely on that pinned version.- If you’re relying on corepack auto-switching (Node 18+), consider adding a note in the root
README.mdto guide developers.patches/tldraw@2.3.0.patch (4)
39-59: ESM re-exports look correct
index.mjsimports and re-exports the new helpers. Good alignment with the declared types.
118-149: MenuItem: style pass-through to icon LGTMPassing
styledown toTldrawUiButtonIconis consistent and enables external sizing/theming. Looks good.
1-35: Unable to locate CJS entry file for patch applicationI couldn’t find any
dist-cjs/index.*files in the repository—these are generated only in the installedtldrawpackage (e.g. undernode_modules/tldraw/dist-cjs). Please manually verify:• The exact CJS entry filename (commonly
index.jsorindex.cjs) undernode_modules/tldraw/dist-cjs.
• That you’re exporting bothregisterDefaultSideEffectsandregisterDefaultExternalContentHandlersin that file, for example:Object.defineProperty(exports, "registerDefaultExternalContentHandlers", { enumerable: true, get: function () { return require("./lib/defaultExternalContentHandlers.js").registerDefaultExternalContentHandlers; } }); Object.defineProperty(exports, "registerDefaultSideEffects", { enumerable: true, get: function () { return require("./lib/defaultSideEffects.js").registerDefaultSideEffects; } });Once you’ve confirmed the path, please apply the corresponding CJS export additions so consumers don’t get
undefinedat runtime.
82-95: Please verify the TypeScript declaration forTldrawUiButtonIconincludes thestylepropProp‐drilling
stylethroughTldrawUiButtonIconis correct, but I wasn’t able to locate a corresponding.d.tsentry in the compiled outputs. Could you confirm (or add) the public declaration to includestyle, for example:export declare function TldrawUiButtonIcon(props: { icon: React.ReactNode; small?: boolean; invertIcon?: boolean; style?: React.CSSProperties; }): JSX.Element;This will prevent TypeScript errors for external consumers.
packages/database/package.json (1)
31-36: Workspace and Supabase tooling placement looks correctMarking internal packages as
workspace:*and keeping Supabase libs in devDependencies matches this package’s schema/migration role.packages/ui/package.json (1)
26-28: Good: switch to workspace: for internal config packages*This aligns with pnpm workspaces and avoids version drift.
apps/obsidian/package.json (2)
36-39: Confirm Obsidian runtime React constraintsUsing
react/react-domfromcatalog:obsidianis fine if versions match Obsidian’s supported runtime. Please confirm the catalog pins versions that are safe for the Obsidian desktop runtime currently in use.
19-23: Catalog entries confirmed for Obsidian
Thecatalog:obsidianreferences for React typings are already defined inpnpm-workspace.yaml:
catalogs.obsidian.react: 19.0.0catalogs.obsidian.react-dom: 19.0.0No further action required.
apps/website/package.json (2)
16-18: Good workspace wiringSwitching to
workspace:*for@repo/databaseand@repo/uiis correct for pnpm workspaces.
41-55: Types and MD stack look goodTypings for hast/mdast and the remark/unified stack are scoped correctly to devDependencies.
apps/roam/package.json (6)
15-17: Good shift to workspace: for internal packages*Using workspace protocol for @repo/* aligns with pnpm workspaces and will improve local linking and dedupe.
23-23: dotenv in devDependencies makes sense for script-time envUsing dotenv for tsx-driven scripts (dev/build/deploy/publish) is appropriate.
34-35: BPJS 3 pinned versions look consistent with Roam extension guidanceUsing @blueprintjs/core 3.x and icons 3.x matches the established UI stack for the Roam extension per team guidance.
38-39: Internal packages via workspace: is correct*Good move; this ensures local consistency with the monorepo and reduces version skew.
45-47: @vercel/blob in a browser extension: confirm intended usage and envIf used client-side, ensure you’re invoking the browser client APIs only and not server-side helpers that expect Node/Edge runtimes. Also verify required env vars and runtime configs don’t leak into the extension bundle.
52-56: Runtime utility additions look reasonabledate-fns, eventemitter3, fuzzy, lodash.isequal are lightweight and commonly used. Nothing stands out.
.github/workflows/roam-pr.yaml
Outdated
|
|
||
| - name: Install Dependencies | ||
| run: npm ci | ||
| run: pnpm install --prefer-frozen-lockfile |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use --frozen-lockfile for deterministic PR deploys.
Match CI’s stricter install to avoid silent lockfile updates during deploys.
- run: pnpm install --prefer-frozen-lockfile
+ run: pnpm install --frozen-lockfile📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: pnpm install --prefer-frozen-lockfile | |
| run: pnpm install --frozen-lockfile |
🤖 Prompt for AI Agents
In .github/workflows/roam-pr.yaml around line 38, the workflow uses "pnpm
install --prefer-frozen-lockfile" which can still update the lockfile; replace
the flag with "--frozen-lockfile" so the install fails if the lockfile is out of
sync (ensuring deterministic PR deploys and matching CI behavior).
apps/obsidian/package.json
Outdated
| "publish": "tsx scripts/publish.ts --version 0.1.0" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove hardcoded version from publish script
Hardcoding --version 0.1.0 conflicts with README examples and prevents publishing arbitrary versions via pnpm run publish -- --version ....
- "publish": "tsx scripts/publish.ts --version 0.1.0"
+ "publish": "tsx scripts/publish.ts"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "publish": "tsx scripts/publish.ts --version 0.1.0" | |
| }, | |
| "publish": "tsx scripts/publish.ts" | |
| }, |
🤖 Prompt for AI Agents
In apps/obsidian/package.json around lines 12-13, the publish script hardcodes
"--version 0.1.0" which prevents forwarding arbitrary versions; remove the
hardcoded "--version 0.1.0" so the script becomes simply the publish command
(allowing callers to pass -- --version X) or otherwise accept
npm_package_version inside the script; update the publish entry to not include a
fixed version and verify pnpm run publish -- --version X works as expected.
| **🧪 Pre-release** (automatic detection) | ||
| - **Triggers**: Any version with `alpha` or `beta` suffixes | ||
| - **GitHub**: Marked as "Pre-release" | ||
| - **GitHub**: Marked as "Pre-release" | ||
| - **Main Branch**: Not updated (keeps stable code in main) |
There was a problem hiding this comment.
Inconsistent “beta” release status between sections
Above you state pre-releases (alpha/beta) are marked as “Pre-release”, but the table shows “GitHub Release: ✅ Stable” for beta. Align to “🧪 Pre-release” for beta rows.
-| `x.y.z-beta.n` | External Pre-release | ✅ Yes | ❌ No | ✅ Stable | `1.0.0-beta.1` |
+| `x.y.z-beta.n` | External Pre-release | ✅ Yes | ❌ No | 🧪 Pre-release | `1.0.0-beta.1` |Also applies to: 101-106
🧰 Tools
🪛 LanguageTool
[grammar] ~55-~55: There might be a mistake here.
Context: ...Pre-release** (automatic detection) - Triggers: Any version with alpha or `...
(QB_NEW_EN)
[grammar] ~56-~56: There might be a mistake here.
Context: ...ion with alpha or beta suffixes - GitHub: Marked as "Pre-release" - **Mai...
(QB_NEW_EN)
[grammar] ~57-~57: There might be a mistake here.
Context: ...GitHub: Marked as "Pre-release" - Main Branch: Not updated (keeps stable ...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In apps/obsidian/scripts/README.md around lines 54 to 57 (and also apply same
fix to lines 101–106), the table entry incorrectly marks beta releases as
"GitHub Release: ✅ Stable" while the section above defines beta as a
pre-release; update the beta rows in the table to mark GitHub releases as "🧪
Pre-release" (or equivalent pre-release indicator) to match the documented
behavior and keep the wording consistent across both places.
There was a problem hiding this comment.
out of PR scope. This was only included because of automated spacing adjustments.
There was a problem hiding this comment.
@maparent Understood! You're absolutely right that this documentation inconsistency is out of scope for the PNPM migration PR.
Would you like me to create a follow-up issue to track the beta release documentation inconsistency for future cleanup?
| ### Using pnpm script from obsidian directory | ||
| ```bash | ||
| cd apps/obsidian | ||
| npm run publish -- --version 1.0.0-beta.1 | ||
| pnpm run publish -- --version 1.0.0-beta.1 | ||
| ``` |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Publish script invocation may pass duplicate --version
The README suggests pnpm run publish -- --version ..., but the package.json script currently hardcodes --version 0.1.0. This will pass two versions. Either remove the default in package.json or change the README to call tsx directly.
If you choose to remove the hardcoded version (preferred), apply this in apps/obsidian/package.json (see separate comment there).
🏁 Script executed:
#!/bin/bash
# Inspect the "publish" script in apps/obsidian/package.json to verify if it hardcodes a version flag
grep -R '"publish"' -n apps/obsidian/package.json -A5Length of output: 218
Remove hardcoded --version from the Obsidian publish script
We’ve confirmed that apps/obsidian/package.json defines:
"publish": "tsx scripts/publish.ts --version 0.1.0"
but the README instructs users to run:
pnpm run publish -- --version 1.0.0-beta.1which results in two --version flags being passed.
To resolve this, please remove the hardcoded --version in package.json so that the version is supplied only via the CLI. For example:
• File: apps/obsidian/package.json
Change:
- "publish": "tsx scripts/publish.ts --version 0.1.0"
+ "publish": "tsx scripts/publish.ts"After this change, the README snippet remains correct and users can set any version via:
cd apps/obsidian
pnpm run publish -- --version 1.0.0-beta.1🤖 Prompt for AI Agents
In apps/obsidian/package.json (remove hardcoded --version so README and CLI flag
don't collide), replace the publish script that currently includes a fixed
"--version x.y.z" with a script that simply runs the publish script file (e.g.,
"tsx scripts/publish.ts") so the version is supplied only via the CLI; update
package.json accordingly and verify pnpm run publish -- --version <value> works
as expected.
There was a problem hiding this comment.
Someone more familiar with that part of the codebase should implement a better solution, or mark it as needed for later.
There was a problem hiding this comment.
@maparent Understood! This is indeed a technical debt item that can be addressed later. The current hardcoded version won't break functionality, just creates a confusing developer experience.
Would you like me to create a GitHub issue to track this improvement? I can document the problem (duplicate --version flags when using the README's suggested command) and the proposed solution (remove hardcoded version from package.json) so it doesn't get lost.
🧠 Learnings used
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/obsidian.mdc:0-0
Timestamp: 2025-07-19T22:34:16.794Z
Learning: Applies to apps/obsidian/package.json : Prefer existing dependencies from package.json when adding or using dependencies in the Obsidian plugin
| "paths": { | ||
| "~/*": ["./src/*"], | ||
| "react": ["./node_modules/@types/react"], | ||
| "react-dom": ["./node_modules/@types/react-dom"] | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove paths remaps for react/react-dom; use types instead to avoid breaking runtime/bundler resolution
Mapping module specifiers react and react-dom to @types/* via compilerOptions.paths can make bundlers or IDEs resolve to type packages at build time, which don’t exist at runtime. This is a common cause of “Cannot find module @types/react” or silent mis-resolves.
Apply this diff to the paths block:
"paths": {
- "~/*": ["./src/*"],
- "react": ["./node_modules/@types/react"],
- "react-dom": ["./node_modules/@types/react-dom"]
+ "~/*": ["./src/*"]
}Then, declare the typings at the compilerOptions level (outside the paths object):
{
"compilerOptions": {
// ...
"types": ["react", "react-dom"]
}
}Also, if any JSX/TSX is present, ensure it’s included; otherwise TSX files won’t be type-checked:
- "include": ["**/*.ts"],
+ "include": ["**/*.ts", "**/*.tsx"],Finally, verify your base config (@repo/typescript-config/react-library.json) sets "jsx": "react-jsx" and a compatible module resolution (e.g., "moduleResolution": "bundler" if applicable). If not, we can adjust here.
🏁 Script executed:
#!/bin/bash
echo "## Checking apps/obsidian/tsconfig.json"
grep -n -C3 '"paths"' apps/obsidian/tsconfig.json
grep -n -C3 '"include"' apps/obsidian/tsconfig.json
echo
echo "## Finding base config react-library.json"
FILE=$(find . -type f -path "*/typescript-config/react-library.json" | head -n1)
echo "Found file: $FILE"
grep -n -C2 '"jsx"' "$FILE"
grep -n -C2 '"moduleResolution"' "$FILE"Length of output: 1212
🏁 Script executed:
#!/bin/bash
echo "## Searching for 'types' in react-library.json"
grep -n '"types"' ./packages/typescript-config/react-library.json || echo "no 'types' field in react-library.json"
echo
echo "## Inspecting base.json for moduleResolution and types"
grep -n -C2 '"moduleResolution"' ./packages/typescript-config/base.json || echo "no 'moduleResolution' in base.json"
grep -n '"types"' ./packages/typescript-config/base.json || echo "no 'types' field in base.json"Length of output: 782
🏁 Script executed:
#!/bin/bash
echo "## Checking for TSX files in apps/obsidian"
find apps/obsidian -type f -name "*.tsx" | head -n 5 || echo "no tsx files found"
echo
echo "## Searching 'React' usage in .ts and .tsx"
grep -R -n --include '*.ts' --include '*.tsx' 'React' apps/obsidian/src || echo "no React usage found"Length of output: 1412
Fix tsconfig paths remapping and include React typings
Mapping react/react-dom via compilerOptions.paths to @types/* will break runtime or bundler resolution (these type‐only packages don’t exist at runtime). Instead, remove those path remaps, declare the typings with compilerOptions.types, and ensure your project includes .tsx files for type checking.
Please apply the following changes to apps/obsidian/tsconfig.json:
• In the compilerOptions block:
"compilerOptions": {
"baseUrl": ".",
"rootDir": ".",
- "paths": {
- "~/*": ["./src/*"],
- "react": ["./node_modules/@types/react"],
- "react-dom": ["./node_modules/@types/react-dom"]
- },
+ "paths": {
+ "~/*": ["./src/*"]
+ },
+ "types": ["react", "react-dom"],
// ...• Update your include to cover TSX:
- "include": ["**/*.ts"],
+ "include": ["**/*.ts", "**/*.tsx"],Finally, verify your base config at packages/typescript-config/react-library.json has:
{
"compilerOptions": {
"jsx": "react-jsx"
// no explicit "types" here; we’re adding them per‐project
}
}and that your shared base.json uses a compatible module resolution (it currently has "moduleResolution": "NodeNext"). If your bundler targets web‐style specifiers, you may opt for "moduleResolution": "bundler", but NodeNext works for Node/Electron environments.
These fixes will restore correct runtime imports and ensure TSX files and React types are properly recognized.
🤖 Prompt for AI Agents
In apps/obsidian/tsconfig.json around lines 15-19, the tsconfig incorrectly
remaps "react" and "react-dom" to @types via compilerOptions.paths which breaks
runtime/bundler resolution; remove those "react" and "react-dom" path mappings
from "paths", add a "types" entry under compilerOptions that includes "react"
(and "react-dom" if needed) so typings are declared at compile time, and change
the top-level "include" to cover TSX (e.g., include "**/*.tsx" alongside
existing globs) so React components are type-checked; finally verify
packages/typescript-config/react-library.json sets "jsx": "react-jsx" and that
shared base.json uses a compatible moduleResolution (NodeNext or bundler) as
described.
| "paths": { | ||
| "@repo/*": ["../../packages/*"], | ||
| "~/*": ["./src/*"], | ||
| "react": ["./node_modules/@types/react"], | ||
| "react-dom": ["./node_modules/@types/react-dom"] | ||
| } |
There was a problem hiding this comment.
Remove react/react-dom path aliases — they shadow runtime modules and will break bundler resolution
Mapping "react" and "react-dom" to @types is an anti-pattern; it makes TypeScript resolve imports to .d.ts files and can cause runtime/bundler failures. Prefer declaring types via devDependencies and compilerOptions.types.
"paths": {
"@repo/*": ["../../packages/*"],
"~/*": ["./src/*"],
- "react": ["./node_modules/@types/react"],
- "react-dom": ["./node_modules/@types/react-dom"]
+ },
+ "types": ["react", "react-dom"]
- }
+ Also ensure @types/react and @types/react-dom are present in the relevant package.json devDependencies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "paths": { | |
| "@repo/*": ["../../packages/*"], | |
| "~/*": ["./src/*"], | |
| "react": ["./node_modules/@types/react"], | |
| "react-dom": ["./node_modules/@types/react-dom"] | |
| } | |
| "paths": { | |
| "@repo/*": ["../../packages/*"], | |
| "~/*": ["./src/*"] | |
| }, | |
| "types": ["react", "react-dom"] |
🤖 Prompt for AI Agents
In apps/roam/tsconfig.json around lines 19-24, remove the "react" and
"react-dom" path mappings that point to @types to avoid shadowing runtime
modules; instead delete those two entries from "paths", ensure @types/react and
@types/react-dom are listed in the package's devDependencies (and install them),
and if you need global typing exposure add them via compilerOptions.types or
rely on node_modules resolution rather than path aliases so bundlers resolve the
real runtime packages.
| "next.config.ts", | ||
| "**/*.ts", | ||
| "**/*.tsx", | ||
| "scripts/*tx", |
There was a problem hiding this comment.
Typo in include pattern excludes scripts.
"scripts/*tx" won’t match .ts or .tsx files as intended.
Apply one of these:
- "scripts/*tx",
+ "scripts/**/*.ts",
+ "scripts/**/*.tsx",or the brace form:
- "scripts/*tx",
+ "scripts/**/*.{ts,tsx}",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "scripts/*tx", | |
| "scripts/**/*.{ts,tsx}", |
🤖 Prompt for AI Agents
In apps/website/tsconfig.json around line 16 the include pattern "scripts/*tx"
is a typo and won't match .ts or .tsx files; update it to either explicit
patterns "scripts/*.ts" and "scripts/*.tsx" or use the brace/glob form
"scripts/*.{ts,tsx}" so .ts and .tsx files in the scripts directory are
correctly included.
There was a problem hiding this comment.
There are no .tsx files in scripts.
There was a problem hiding this comment.
🧩 Analysis chain
@maparent Thank you for the clarification! You're absolutely right. Let me check the actual files in the scripts directory and provide a more accurate suggestion.
Based on your feedback that there are no .tsx files in scripts, the pattern should be updated to:
- "scripts/*tx",
+ "scripts/*.ts",This will correctly match only the TypeScript files that actually exist in the scripts directory, without unnecessarily including .tsx patterns.
🏁 Script executed:
#!/bin/bash
# Check what files exist in the apps/website/scripts directory
fd . apps/website/scripts --type f
Length of output: 107
@maparent You're absolutely right! Looking at the directory structure, there is no scripts directory in apps/website at all. Since the pattern "scripts/*tx" isn't matching any existing files, it should simply be removed entirely:
- "scripts/*tx",This cleans up the configuration by removing an unused include pattern. Thank you for the correction - it's much cleaner to remove unnecessary patterns rather than fix patterns that don't need to exist.
🏁 Script executed:
#!/bin/bash
# Let's check if there are any script-related files in the apps/website directory
fd -e ts -e tsx . apps/website | grep -i script
Length of output: 0
@maparent Perfect! The search confirms there are no script-related files in the apps/website directory. Since the "scripts/*tx" pattern isn't matching any existing files, the cleanest solution is to remove it entirely:
- "scripts/*tx",This removes the unused include pattern from the TypeScript configuration. Thank you for the clarification - it's much better to remove unnecessary configuration rather than fix patterns that aren't needed!
🧠 Learnings used
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Prettier with the project's configuration
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to apps/website/**/*.{tsx,jsx,ts,js} : Use Tailwind CSS for styling where possible
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Maintain consistent naming conventions: UPPERCASE for constants
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Maintain consistent naming conventions: PascalCase for components and types
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Maintain consistent naming conventions: camelCase for variables and functions
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx} : Avoid `any` types when possible
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#377
File: packages/ui/tsconfig.json:5-7
Timestamp: 2025-08-27T02:23:45.696Z
Learning: When reviewing TypeScript configuration changes, check if settings like "declaration" are inherited from base configs before suggesting to add them explicitly. Many monorepo packages extend base configurations that already include necessary compiler options.
packages/database/package.json
Outdated
| "check-types": "pnpm run lint && supabase stop && pnpm run dbdiff", | ||
| "lint": "eslint . && tsx scripts/lint.ts", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate “supabase stop” and side effects in check-types
check-types calls supabase stop, and dbdiff already does the same. Remove the extra call to avoid redundant side effects in a types check.
-"check-types": "pnpm run lint && supabase stop && pnpm run dbdiff",
+"check-types": "pnpm run lint && pnpm run dbdiff",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "check-types": "pnpm run lint && supabase stop && pnpm run dbdiff", | |
| "lint": "eslint . && tsx scripts/lint.ts", | |
| ++ b/packages/database/package.json | |
| @@ scripts | |
| "check-types": "pnpm run lint && pnpm run dbdiff", | |
| "lint": "eslint . && tsx scripts/lint.ts", |
🤖 Prompt for AI Agents
In packages/database/package.json around lines 17 to 18, the "check-types"
script redundantly invokes "supabase stop" even though the "dbdiff" script
already performs that action; remove the extra "supabase stop" from the
"check-types" script so it simply runs "pnpm run lint && pnpm run dbdiff" to
avoid duplicate side effects.
| "@repo/typescript-config": "workspace:*", | ||
| "tailwindcss": "^3.4.1", | ||
| "tailwindcss-animate": "^1.0.7" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make Tailwind and its plugin peer dependencies of the config package.
Since consumers import this config, Tailwind and plugins must resolve at the consumer app. Keeping them only as devDependencies risks pruning in production installs (NODE_ENV=production), causing "Cannot find module 'tailwindcss-animate'" at build time.
Apply this diff to narrow devDependencies and avoid duplication:
"devDependencies": {
- "@repo/typescript-config": "workspace:*",
- "tailwindcss": "^3.4.1",
- "tailwindcss-animate": "^1.0.7"
+ "@repo/typescript-config": "workspace:*"
}Add the following block outside the changed range to declare peers (and keep versions aligned):
{
"peerDependencies": {
"tailwindcss": "^3.4.1",
"tailwindcss-animate": "^1.0.7"
},
"devDependencies": {
"@repo/typescript-config": "workspace:*",
"tailwindcss": "^3.4.1",
"tailwindcss-animate": "^1.0.7"
}
}Please confirm that each consumer package (e.g., apps/website, apps/roam) lists tailwindcss and tailwindcss-animate in its dependencies to satisfy the peer requirements. I can generate a script to check and open follow-up PRs if needed.
🏁 Script executed:
#!/bin/bash
# Verify that each consumer package includes tailwindcss and tailwindcss-animate in its dependencies
for pkg in apps/*; do
PACKAGE_JSON="$pkg/package.json"
if [ -f "$PACKAGE_JSON" ]; then
echo "Checking $pkg"
echo "- tailwindcss entries:"
grep -E '"tailwindcss"' -n "$PACKAGE_JSON" || echo " tailwindcss not found"
echo "- tailwindcss-animate entries:"
grep -E '"tailwindcss-animate"' -n "$PACKAGE_JSON" || echo " tailwindcss-animate not found"
echo ""
fi
doneLength of output: 1519
Consumer packages missing required Tailwind dependencies
Our verification shows that while apps/obsidian correctly declares both Tailwind and the animate plugin, the other consumers are incomplete:
-
apps/obsidian
- tailwindcss (“^3.4.17”) present (line 29)
- tailwindcss-animate (“^1.0.7”) present (line 39)
-
apps/website
- tailwindcss (“^3.4.16”) present (line 53)
- tailwindcss-animate missing
-
apps/roam
- tailwindcss missing
- tailwindcss-animate missing
To satisfy the peerDependencies declared in packages/tailwind-config/package.json, please update each consumer’s dependencies block. For example:
// In apps/website/package.json
"dependencies": {
// …existing deps…
"tailwindcss": "^3.4.16",
+ "tailwindcss-animate": "^1.0.7"
}// In apps/roam/package.json
"dependencies": {
// …existing deps…
+ "tailwindcss": "^3.4.17",
+ "tailwindcss-animate": "^1.0.7"
}Ensure the versions align with the peer ranges in the config package (^3.4.1 for Tailwind and ^1.0.7 for the plugin), or update those peer ranges if you need to bump versions across all packages. This will prevent “Cannot find module ‘tailwindcss-animate’” errors in production builds.
🤖 Prompt for AI Agents
In packages/tailwind-config/package.json around lines 10–12, the package
declares peerDependencies for tailwind and the animate plugin but consumer apps
are missing those deps; update apps/website and apps/roam package.json
dependency blocks to include "tailwindcss": "^3.4.1" and "tailwindcss-animate":
"^1.0.7" (or bump both consumers to versions that match any adjusted peer
ranges), then run install/lockfile update so builds can resolve the modules;
alternatively, if you intend to use newer versions, update the peerDependencies
in packages/tailwind-config/package.json to match the chosen versions and
propagate that change to all consumers.
There was a problem hiding this comment.
@mdroidian not sure about whether tailwindcss and friends should be dependencies or devDependencies? We see both in obsidian.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| catalogs: | ||
| obsidian: | ||
| react: 19.0.0 | ||
| react-dom: 19.0.0 | ||
| "@types/react": ^19.0.0 | ||
| "@types/react-dom": ^19.0.0 | ||
| roam: | ||
| react: 18.2.0 | ||
| react-dom: 18.2.0 | ||
| "@types/react": 18.2.0 | ||
| "@types/react-dom": 18.2.0 | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Roam catalog must stay on React 17 per platform constraints
Per our prior constraint for apps/roam, React 18+ APIs (e.g., createRoot) are not allowed. The current roam catalog sets React 18.2.0, which conflicts with both that constraint and your patch in @tldraw/editor explicitly targeting React 17.
Align the roam catalog with React 17:
catalogs:
obsidian:
react: 19.0.0
react-dom: 19.0.0
"@types/react": ^19.0.0
"@types/react-dom": ^19.0.0
roam:
- react: 18.2.0
- react-dom: 18.2.0
- "@types/react": 18.2.0
- "@types/react-dom": 18.2.0
+ react: 17.0.2
+ react-dom: 17.0.2
+ "@types/react": ^17.0.0
+ "@types/react-dom": ^17.0.0The project-specific overrides (e.g., "roam>react": catalog:roam) are correct; they’ll enforce this where needed.
Confirm the roam app truly consumes the roam catalog and does not import react 18/19 via transitive deps:
Also applies to: 33-45
🏁 Script executed:
#!/bin/bash
# Show react* versions declared in apps/roam and effective overrides
rg -nI -C2 '"react(-dom)?":' apps/roam/package.json || true
rg -nI -C2 '"@types/react' apps/roam/package.json || true
# Ensure no imports from react-dom/client in our code
rg -nI -n --type=ts --type=tsx -g 'apps/roam/**' "react-dom/client"Length of output: 739
To ensure we aren’t missing any React 18+ entry points, let’s broaden the search patterns:
🏁 Script executed:
#!/bin/bash
# Look for any usage of the new React 18 APIs in apps/roam
rg -nI -n -g 'apps/roam/**/*.{ts,tsx,js,jsx}' "react-dom/client" || true
rg -nI -n -g 'apps/roam/**/*.{ts,tsx,js,jsx}' "createRoot" || trueLength of output: 157
Let’s pinpoint where that "react-dom/client" string occurs to understand its context:
🏁 Script executed:
#!/bin/bash
# Show filename and surrounding lines for the react-dom/client import
rg -H -n -C3 -g 'apps/roam/**/*.{ts,tsx,js,jsx}' "react-dom/client" || trueLength of output: 510
Enforce React 17 in Roam catalog
- Verified
apps/roam/package.jsondeclares bothreactandreact-domvia"catalog:roam", so the versions inpnpm-workspace.yamldrive the actual install. - Confirmed no calls to
createRootacrossapps/roam/**/*.{ts,tsx,js,jsx}—Roam code does not use the React 18 entrypoint. - Noted
apps/roam/scripts/compile.tslists"react-dom/client"in its externals array, but this is a bundler directive and not an import or runtime usage.
Please update pnpm-workspace.yaml to pin Roam to React 17:
catalogs:
roam:
- react: 18.2.0
- react-dom: 18.2.0
- "@types/react": 18.2.0
- "@types/react-dom": 18.2.0
+ react: 17.0.2
+ react-dom: 17.0.2
+ "@types/react": ^17.0.0
+ "@types/react-dom": ^17.0.0This will enforce the React 17 constraint for the Roam app.
🤖 Prompt for AI Agents
In pnpm-workspace.yaml around lines 11 to 22 the Roam catalog is pinned to React
18 (react/react-dom 18.2.0); update the Roam entries to pin React 17 by setting
react and react-dom to a 17.x version (e.g., 17.0.2) and adjust the
corresponding type packages to the 17.x line (e.g., "@types/react": ^17.0.0 and
"@types/react-dom": ^17.0.0) so the Roam app installs and uses React 17 as
required.
52d69f4 to
c2f63df
Compare
Try something like const PostHogPageView: React.ComponentType = dynamic(
() => Promise.resolve(PostHogPageViewInner),
{
ssr: false,
},
); |
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
2 times, most recently
from
ea0540a to
7bdfbec
Compare
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
6 times, most recently
from
503f13b to
625fa24
Compare
.github/workflows/ci.yaml
Outdated
| - uses: pnpm/action-setup@v4 | ||
| name: Install pnpm | ||
| with: | ||
| version: 10.15.0 |
There was a problem hiding this comment.
Reminder: pnpm 10 requires ENABLE_EXPERIMENTAL_COREPACK=1 in vercel. We'll have to set for production upon merge.
There was a problem hiding this comment.
I did set it globally in vercel a few days ago btw.
| "lint": "eslint .", | ||
| "lint:fix": "eslint . --fix", | ||
| "publish": "tsx scripts/publish-obsidian.ts", | ||
| "publish": "tsx scripts/publish.ts --version 0.1.0", |
There was a problem hiding this comment.
as discussed: the publish script requires a version. This probably had issues before.
| "importHelpers": true, | ||
| "isolatedModules": true, | ||
| "strictNullChecks": true, | ||
| "paths": { |
There was a problem hiding this comment.
For the record: changes to these files were necessary at some point, but not anymore. Could revert. But It think declaring types explicitly is useful.
| "types": ["react", "react-dom"], | ||
| "paths": { | ||
| "~/*": ["./src/*"], | ||
| "react": ["./node_modules/@types/react"], |
There was a problem hiding this comment.
This is the nuclear option; always use the react types.
maparent
force-pushed
the
eng-757-experiment-with-pnpm
branch
3 times, most recently
from
81e0a27 to
8181a98
Compare
|
Interesting option with more recent pnpm to mitigate the risk of dependency chain attacks: |
* Package files have to be more explicit in their dependencies. * pnpm workspace definition. * nuclear option: Use the react->@types/react@18 for tsc. * Patch for @blueprintjs/core@3.50.4 and adjust versioning for dependencies. (#425) --------- Co-authored-by: Michael Gartner <mclicks@gmail.com> some coderabbit suggestions, update pnpm to 10.15.1, remove some unneeded changes, comments
mdroidian
force-pushed
the
eng-757-experiment-with-pnpm
branch
from
8181a98 to
a6b59ba
Compare
* Convert to PNPM. * Package files have to be more explicit in their dependencies. * pnpm workspace definition. * nuclear option: Use the react->@types/react@18 for tsc. * Patch for @blueprintjs/core@3.50.4 and adjust versioning for dependencies. (#425) --------- Co-authored-by: Michael Gartner <mclicks@gmail.com> some coderabbit suggestions, update pnpm to 10.15.1, remove some unneeded changes, comments * regenerate pnpm-lock after rebase * include eslint as specific dependency in various package.json. Put eslint in catalog. * patch @blueprint/select. pnpm-workspace.yaml got normalized in the process --------- Co-authored-by: Michael Gartner <mclicks@gmail.com>
https://linear.app/discourse-graphs/issue/ENG-757/experiment-with-pnpm
Context: having trouble with adding
@blueprintjs/tables. Plus was mentioned in passing in dev conversation. So I thought I'd try to build the project with pnpm, just to see how hard it was. Some things are definitely simpler, but some things require being more explicit.Keeping this PR in draft, as this is another major change that we should not take lightly.
However, I suspect that most of the changes in this PR that are not directly related to pnpm would improve our code base, and I will likely introduce them independently.
Summary by CodeRabbit