Consistently use build/ directory for built code - #1231
ThisIsMissEm wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (21)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe project changes compiled output paths from ChangesBuild output directory migration
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~15 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The build-output migration is internally consistent across runtime, packaging, container, and workflow paths. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 5 files. (12 skipped: 12 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
We've triggered an ultrareview automatically — This cross-cutting build-output rename touches Dockerfiles, CI, tsconfig/jest/eslint configs, and package exports across three packages, so a missed reference or a subtle rootDir/outDir interaction could break builds and production deploys; a deep multi-pass review is worth it.. I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 2m 21s
All reported issues were addressed across 18 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai all the license changes were in accordance with the LICENSE at the root of the repository, so the old |
@ThisIsMissEm You’re right—the license changes are correct if the repository’s root Tip: get faster answers by chatting with cubic’s review copilot in the review UI |
0054be4 to
6433e39
Compare
Rename the TypeScript output directory from transpiled/ to build/ across server, nodejs-instrumentation and @roostorg/coop-types, so every package emits to the same place. - server: outDir, both start scripts, Dockerfile, the e2e workflow, both jest configs, both eslint configs, the e2e fixture constant, and the three ignore files. Also adds rootDir so the layout under outDir is explicit rather than inferred from the common ancestor of the input files. - nodejs-instrumentation: outDir, clean script, Dockerfile, and the start:trace reference from server. Also drops a dead main (nothing resolves this package by name, and the image flattens build/ into /autoinstrumentation, so that path does not exist there) and marks the package private. - @roostorg/coop-types: outDir plus module, typings, files and exports. Consumer-invisible, because exports declares only ".", so deep imports into transpiled/ were never reachable. build/ was already ignored by .gitignore and .dockerignore, so this mostly removes the transpiled entries rather than adding new ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6433e39 to
8a19a84
Compare
| @@ -3,12 +3,12 @@ | |||
| "type": "module", | |||
| "version": "2.4.0", | |||
There was a problem hiding this comment.
I wonder if we should bump the version given the change.
There was a problem hiding this comment.
I wasn't sure how version is happening for the package, hence leaving it. Normally I use release tooling that handles versioning automatically. What would we define the version bump as? major, minor or patch?
There was a problem hiding this comment.
patch I would guess 🤔 it does change license and exports, but may be ok.
This branch has not been deployed
2 participants
Context & Requests for Reviewers
Rename the TypeScript output directory from transpiled/ to build/ across server, nodejs-instrumentation and @roostorg/coop-types, so every package emits to the same place.
This is a precursor to being able to use import maps, and normally just how the build output of
tscis structured.Changes:
build/ was already ignored by .gitignore and .dockerignore, so this mostly removes the transpiled entries rather than adding new ones.
I also caught a few cases where the package.json
licensewas set to ISC despite the entire project being Apache-2.0There will be a follow up PR that makes the
server/build/hermetic, such that thepackage.jsonis inside thebuild/directory in the docker image, which allows import maps to fully work.Tests
Full suite run, and passed. Also worked with the container to ensure it started up correctly.
(Optional) Rollout Plan
n/a
Checklist
Only check items that apply to this PR; leave the rest unchecked.
If you changed anything user-facing (i.e. user interface or APIs):
Did you update related docs?
If the change is notable (refer to Keep a Changelog conventions):
Did you update CHANGELOG.md?
If you changed
db/src/scripts/**and usedCREATE TABLE,ADD COLUMN, orALTER COLUMN:Are as many columns marked
NOT NULLas possible? If some columns can sometimes be null depending on other columns, are thereCHECKconstraints capturing those relationships, and are these also reflected using unions in the associated Kysely types?If you added a new signal in
server/services/signalsService/signals/**:Did you classify every error case as a permanent error (
SignalPermanentError, no retry) or a normal error (retryable)? Any case where the signal can't determine a score should be aSignalPermanentError.Summary by CodeRabbit
Build and Deployment
builddirectory.Package Updates