ENG-847 include, do not build dependencies. #431
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
maparent
force-pushed
the
eng-847-temp-fix-compile-roam-against-database-ts-to-get-env-vars
branch
from
09b8545 to
698e302
Compare
maparent
changed the base branch from
main
to
eng-848-update-github-workflow-package-dependencies
maparent
changed the title
|
Note: This obviates the need for ENG-820 |
|
The remaining deploy issue is handled in ENG-821, which is now rebased above this. |
| "tasks": { | ||
| "build": { | ||
| "dependsOn": ["^build"], | ||
| "dependsOn": ["@repo/database#genenv"], |
| "persistent": true, | ||
| "inputs": ["$TURBO_DEFAULT$", ".env*"], | ||
| "dependsOn": ["^build"] | ||
| "dependsOn": ["@repo/database#genenv"] |
maparent
force-pushed
the
eng-847-temp-fix-compile-roam-against-database-ts-to-get-env-vars
branch
from
8db1d56 to
252e57b
Compare
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughExports in database, ui, and utils packages were repointed from dist outputs to TypeScript source files. Types-related mappings were simplified/removed. Type-check scripts switched to no-emit mode. Turbo tasks were restructured to add a new genenv step and update dev/build dependencies; new dev aliases for roam and website were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Turbo as Turbo
participant DB as @repo/database
participant App as App packages (roam/website/others)
Dev->>Turbo: turbo run dev
Turbo->>DB: run genenv (inputs: SUPABASE_USE_DB)
DB-->>Turbo: outputs .env*
Turbo->>App: start dev (with .env ready)
Note over Turbo,App: roam#dev / website#dev run with @repo/database#dev
Dev->>Turbo: turbo run build
Turbo->>DB: run genenv
DB-->>Turbo: outputs .env*
Turbo->>App: run build (env pre-generated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/package.json (1)
14-26: Remove or fix typesVersions in packages/ui/package.json — it points to non-existent dist/*.d.tspackages/ui/tsconfig.json has no "declaration: true" and packages/ui/dist is absent, so the current typesVersions mapping targets files that aren't emitted and will break TS/editor resolution. Either remove the typesVersions block, or enable declaration output into dist/*.d.ts and keep the mappings, or update typesVersions to valid src paths only if those source paths actually exist and you intend source-based mapping.
File: packages/ui/package.json (lines 14–26)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/package.json(2 hunks)packages/ui/package.json(2 hunks)packages/utils/package.json(1 hunks)turbo.json(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-27T02:23:45.696Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#377
File: packages/ui/tsconfig.json:5-7
Timestamp: 2025-08-27T02:23:45.696Z
Learning: Don't assume package structure or suggest main/types entries in package.json without verifying the files exist. UI packages and component libraries may not have a single main entry point and can be designed to export components individually or through subpath exports.
Applied to files:
packages/ui/package.jsonpackages/utils/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.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 **/*.{ts,tsx,js,jsx} : Use Prettier with the project's configuration
Applied to files:
packages/ui/package.json
📚 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/package.json
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.
Applied to files:
packages/database/package.json
📚 Learning: 2025-09-01T19:09:07.052Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-09-01T19:09:07.052Z
Learning: User prefers to avoid making types explicit in each require statement due to anti-DRY concerns when working with module imports in TypeScript.
Applied to files:
packages/database/package.json
🔇 Additional comments (8)
packages/utils/package.json (2)
11-12: LGTM on stricter type-check.Switch to tsc --noEmit --skipLibCheck is appropriate for CI type checks.
6-8: Wildcard export to raw .ts files can break consumers — confirm transpilation or publish compiled JS.
- Action: Ensure Next.js (transpilePackages) or Vite (optimizeDeps/esbuild) transpiles @repo/utils, or change packages/utils.exports to point to built JS (dist).
- Verification summary: automated scan was inconclusive — rg errored when scanning apps/website/next.config.ts; jq returned null (no moduleResolution/composite/references) for: apps/obsidian/tsconfig.json, apps/roam/tsconfig.json, packages/database/tsconfig.json, packages/tailwind-config/tsconfig.json, packages/ui/tsconfig.json, packages/utils/tsconfig.json.
- Note: sandbox lacks tsc; run local/CI checks or provide configs for re-verification.
packages/ui/package.json (1)
32-33: LGTM on no-emit type check.packages/database/package.json (2)
7-7: Switch to exporting TS sources: verify consumers transpile @repo/database.Same concern as utils/ui; ensure downstream apps handle TS from workspace packages.
Reuse the transpilePackages/Vite checks from utils.
30-31: LGTM on no-emit type check.turbo.json (3)
43-46: Gating build on @repo/database#genenv makes sense.Good to ensure .env* exists before builds.
62-63: LGTM: dev depends on env generation.
64-69: Don't replace "with" with "dependsOn" — "with" is valid for long‑running dev tasks.TurboRepo supports the "with" key for long‑running tasks (e.g., dev) to run @repo/database#dev alongside roam#dev/website#dev; keep the current config.
Likely an incorrect or invalid review comment.
| "./lib/*": "./src/lib/*.ts", | ||
| "./hooks/*": "./src/hooks/*.ts", | ||
| "./components/*": "./src/components/*.tsx" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Source .ts/.tsx exports: ensure apps transpile external packages.
Same risk as utils; verify Next/Vite transpile @repo/ui and that Storybook (if any) is configured similarly.
Use the same script I shared for utils; additionally check for Storybook:
🏁 Script executed:
#!/bin/bash
fd -t f -a 'main.(js|ts|cjs|mjs|ts|tsx)' apps | rg -n 'storybook' -n || trueLength of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== packages/ui/package.json (summary) ==="
if [ -f packages/ui/package.json ]; then
jq '{name: .name, main: .main, types: .types, exports: .exports, files: .files}' packages/ui/package.json || true
echo "---- full file ----"
sed -n '1,240p' packages/ui/package.json || true
else
echo "packages/ui/package.json not found"
fi
echo
echo "=== Search for storybook references and .storybook dirs ==="
rg -n --hidden --no-ignore -S "storybook" || true
find . -type d -name ".storybook" -print || true
echo
echo "=== Search for Next/Vite transpile-related config/usages ==="
rg -n --hidden --no-ignore -S "transpilePackages|next-transpile-modules|withTM|transpile" || true
rg -n --hidden --no-ignore -S "vite.config|optimizeDeps|noExternal|ssr.noExternal|server.noExternal" || true
echo
echo "=== Show next.config.* files (if any) ==="
find . -type f -name "next.config.*" -print -exec sed -n '1,240p' {} \; || true
echo
echo "=== Show vite.config.* files (if any) ==="
find . -type f -name "vite.config.*" -print -exec sed -n '1,240p' {} \; || true
echo
echo "=== Search for imports referencing a ui package (patterns) ==="
rg -n --hidden --no-ignore -S "from ['\"][^'\"]*ui['\"]|require\\(['\"][^'\"]*ui['\"]\\)" || true
echo
echo "=== Search apps/ for package.json scripts mentioning storybook or next/vite ==="
rg -n --hidden --no-ignore -S '"storybook"|"next"|"vite"' apps || trueLength of output: 3756
Exporting source (.ts/.tsx) from @repo/ui — publish compiled JS or enable per-app transpilation
- packages/ui/package.json — exports target ./src/.ts/.tsx, main/types are null, while typesVersions point at dist/.d.ts; this mismatch requires either shipping compiled JS (dist) and updating main/types/exports or intentionally relying on consumer transpilation.
- apps/website/next.config.ts — no transpilePackages / next-transpile-modules configuration found; Next will not transpile node_modules by default, so consuming the source exports will break.
- No Storybook config or storybook scripts detected; if Storybook is used, configure it to transpile @repo/ui or publish compiled output.
🤖 Prompt for AI Agents
In packages/ui/package.json around lines 10–13, the package currently exports
source files (./src/*.ts/.tsx) while main/types are null and typesVersions point
to dist/*.d.ts, causing a mismatch; fix by either (A) publishing compiled
output: add a build step that outputs JS and .d.ts into dist, set "main" to
dist/index.js and "types" to dist/index.d.ts, update the "exports" map to point
to ./dist/*, include dist in "files" and ensure typesVersions match dist, and
update npmignore/packlist so dist is published; or (B) opt into consumer
transpilation: keep source exports but document this and configure consuming
apps—add next.config.js transpilePackages: ['@repo/ui'] (or
next-transpile-modules) and update Storybook webpack to transpile
@repo/ui—choose one approach and make package.json and build/config changes
consistently.
| "genenv": { | ||
| "inputs": ["SUPABASE_USE_DB"], | ||
| "outputs": [".env*"] | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use “env” for task hashing instead of putting SUPABASE_USE_DB in inputs.
inputs expects files/globs; env should be declared via env for cache keys.
"genenv": {
- "inputs": ["SUPABASE_USE_DB"],
+ "env": ["SUPABASE_USE_DB"],
"outputs": [".env*"]
},📝 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.
| "genenv": { | |
| "inputs": ["SUPABASE_USE_DB"], | |
| "outputs": [".env*"] | |
| }, | |
| "genenv": { | |
| "env": ["SUPABASE_USE_DB"], | |
| "outputs": [".env*"] | |
| }, |
🤖 Prompt for AI Agents
In turbo.json around lines 70 to 73, the "genenv" task incorrectly lists the
SUPABASE_USE_DB variable under "inputs" (which expects files/globs); move
SUPABASE_USE_DB into the task's "env" array so Turbo uses it for task
hashing/caching instead of treating it as an input file. Update the "genenv"
config to remove SUPABASE_USE_DB from "inputs" and add it under "env" so the
cache key includes that environment variable.
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!
Specifically, allows it to be called in a non-dev environment.
maparent
deleted the
eng-847-temp-fix-compile-roam-against-database-ts-to-get-env-vars
branch
* ENG-847 compile roam against db ts * Also make utils and ui co-compile rather than build. Adjust turbo dependencies. * ENG-821 Rewrite createEnv.mts to be more resilient. (#434) Specifically, allows it to be called in a non-dev environment.
https://linear.app/discourse-graphs/issue/ENG-847/temp-fix-compile-roam-against-database-ts-to-get-env-vars
Source issue: by including built dependencies, we were not getting the environment variable substitutions in roam/scripts/compile.ts, and the resulting executable still had references to process.
Summary by CodeRabbit
New Features
Chores