chore: remove example app and update configs#32
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Remove the example/ directory and .trae/ rules. Update CI workflow, biome config, tsconfig, and package.json for fiber architecture.
MarcelOlsen
force-pushed
the
fiber/cleanup
branch
from
760fa11 to
2ef69e5
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 31-32: Rename the GitHub Actions step by updating the name field
for the typecheck step (currently "name: typecheck") to include an emoji prefix
to match other steps (e.g., "🔍 typecheck" or another chosen emoji), leaving the
command ("run: bun run typecheck") unchanged; ensure the new name follows the
same emoji + label pattern used across the workflow so the step list remains
visually consistent.
In `@biome.json`:
- Around line 37-39: The project disables the TypeScript non-null assertion rule
by setting "noNonNullAssertion": "off" in biome.json; revert this global disable
by removing that setting or setting it back to "error"/"warn" to re-enable
checks, and if specific files truly need non-null assertions, add a scoped
override in biome.json targeting those file globs to keep the rule enabled
elsewhere; look for the "noNonNullAssertion" entry in biome.json and either
delete it or change its value, or move it into an "overrides" block that lists
the precise files that require relaxing the rule.
In `@package.json`:
- Around line 48-49: The package.json currently lists
"@typescript/native-preview" under "peerDependencies" which forces consumers to
install an experimental compiler; remove "@typescript/native-preview" from the
"peerDependencies" object and add it to "devDependencies" instead (keeping the
same version string "^7.0.0-dev"), then run your lockfile update (npm/yarn pnpm)
to sync the lockfile; ensure any CI/build scripts or published types do not rely
on it being a runtime peer dependency and adjust package.json metadata if
needed.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockexample/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci.yml.trae/rules/project_rules.mdbiome.jsonexample/README.mdexample/babel.config.jsexample/package.jsonexample/public/index.htmlexample/src/App.jsxexample/src/Counter.jsxexample/src/Modal.jsxexample/src/ReducerDemo.jsxexample/src/RefDemo.jsxexample/src/TodoList.jsxexample/src/index.jsexample/tsconfig.jsonexample/webpack.config.jspackage.jsontsconfig.build.jsontsconfig.json
💤 Files with no reviewable changes (14)
- example/src/Modal.jsx
- example/src/App.jsx
- .trae/rules/project_rules.md
- example/package.json
- example/src/ReducerDemo.jsx
- example/src/Counter.jsx
- example/src/RefDemo.jsx
- example/public/index.html
- example/babel.config.js
- example/src/index.js
- example/webpack.config.js
- example/README.md
- example/src/TodoList.jsx
- example/tsconfig.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
🔇 Additional comments (5)
biome.json (1)
34-36:useLiteralKeysoff - this one I can vibe with.Alright, disabling
useLiteralKeysis actually reasonable when you're working with index signatures or need computed property access. If the codebase hasnoPropertyAccessFromIndexSignaturedisabled (which it does in tsconfig), this makes sense to keep the styles consistent.tsconfig.build.json (1)
14-17: Alright, the path alias setup here is correct.Good call only including
@/*and not@tests/*in the build config since tests are excluded anyway. No point mapping paths you're never gonna use.One thing to note:
verbatimModuleSyntax: falsehere overrides thetruein the base config. This is probably intentional since you need the build to actually emit JS, but just making sure that's a conscious choice and not an accident.The
pathshere completely replaces the parent'spaths(TypeScript doesn't merge these), which is what you want for build isolation.package.json (2)
34-34: Thetsgoscripts look clean.Build and typecheck scripts using
tsgo- straightforward, no complaints here. Just make sure this actually works with your CI environment since Bun needs to be able to find thetsgobinary from the installed package.Also applies to: 38-38
31-31: Okay, you're going ALL IN on the TypeScript 7 migration strategy. This ain't just "living on the edge for fun" — Microsoft dropped the TypeScript 6.0 Beta in Feb 2026 literally framing it as the last release on the JS codebase, with TS7 pivoting fully to the Go rewrite. So@typescript/native-previewwithtsgois the official preview of that future state.But here's what still matters:
- Dev preview versions WILL shift under you — this isn't hypothetical, it's baked into the semver. Your lock file keeps the ship steady, but you're explicitly signaling "we accept breaking changes in patches."
- Your peerDependency creates friction for consumers — they've gotta opt into the same bet. Even with the relaxed
^7.0.0-dev, they're now tethered to your experimental choice.- The migration path is legit, but it's a maintenance tax — you're being an early adopter of the canonical direction, which is respectable, but it means staying close to upstream and ready to adapt when things shift.
If you're intentionally riding the TypeScript 7 wave knowing the bumps ahead? That's a defensible architectural decision. Just make sure the team is aligned on the maintenance burden.
tsconfig.json (1)
28-32: Path aliases look good, and you're doing it right.Love the
@/*and@tests/*setup—gonna kill those../../../../src/whateverimports. And here's the thing: you're already doing this correctly. You're using explicit relative paths (./src/*,./tests/*) withoutbaseUrl, which is exactly how it should work in TypeScript 4.1+.Since TypeScript 4.1,
baseUrlis not required forpathsto work—paths are resolved relative to thetsconfig.jsondirectory by default. And honestly? This becomes even more important with@typescript/native-preview(tsgo), becausebaseUrlisn't fully supported there yet anyway. So your approach here is actually the play.One thing though: remember that TypeScript's
pathsonly affects type-checking and module resolution. Your bundler/runtime still needs to be configured to understand these aliases—otherwise your code compiles fine but blows up at runtime. Make sure whatever you're running (Node with tsconfig paths loader, webpack, vite, etc.) is actually set up to handle@/and@tests/*mappings.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/ci.yml:
- Around line 31-32: The CI workflow currently runs the "🔎 Typecheck" step
after tests; reorder the steps so static checks (the step with name "🔎
Typecheck" that runs "bun run typecheck" and any lint steps) execute before the
test step (e.g., the job/step that runs the test command), by moving the "🔎
Typecheck" step block above the test step block in the job definition so
typecheck/lint fail fast and tests only run on successful static checks.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockexample/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci.yml.trae/rules/project_rules.mdbiome.jsonexample/README.mdexample/babel.config.jsexample/package.jsonexample/public/index.htmlexample/src/App.jsxexample/src/Counter.jsxexample/src/Modal.jsxexample/src/ReducerDemo.jsxexample/src/RefDemo.jsxexample/src/TodoList.jsxexample/src/index.jsexample/tsconfig.jsonexample/webpack.config.jspackage.jsontsconfig.build.jsontsconfig.json
💤 Files with no reviewable changes (14)
- example/babel.config.js
- example/src/ReducerDemo.jsx
- example/src/index.js
- example/src/TodoList.jsx
- example/public/index.html
- example/src/RefDemo.jsx
- example/README.md
- example/webpack.config.js
- example/package.json
- example/src/Modal.jsx
- .trae/rules/project_rules.md
- example/tsconfig.json
- example/src/Counter.jsx
- example/src/App.jsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
📚 Learning: 2025-06-17T12:40:39.737Z
Learnt from: MarcelOlsen
Repo: MarcelOlsen/mini-react PR: 8
File: README.md:701-708
Timestamp: 2025-06-17T12:40:39.737Z
Learning: MarcelOlsen prefers to keep test documentation simple with just one example of running specific test files, rather than listing detailed commands for each test suite in the README.
Applied to files:
package.json
🔇 Additional comments (4)
biome.json (1)
34-39: ScopeuseLiteralKeysinstead of nuking it globally.
Disablingcomplexity.useLiteralKeysat the root level (line 34) is a footgun—you're removing a safety rail across the entire codebase. If a few files actually need computed keys, use anoverridesblock to keep everything else strict. Here's how to do it correctly:♻️ Scope it with overrides instead
"complexity": { - "useLiteralKeys": "off" + "useLiteralKeys": "error" }, + "overrides": [ + { + "includes": ["src/fiber/**"], + "linter": { + "rules": { + "complexity": { + "useLiteralKeys": "off" + } + } + } + } + ]The
noNonNullAssertion: errormove is based though—keep the types honest and force developers to actually think about null safety instead of asserting their way out.tsconfig.build.json (1)
14-17: Dude, the path alias isn't even being used—remove the dead config noise.Lines 15-16 define
@/*in tsconfig.build.json, but your source code doesn't import from@/anywhere. You're using relative imports like"./src/MiniReact"instead. The path alias is just cruft sitting there in the config file—if you're not using it, drop it. Keep your TypeScript config clean and intentional. Either commit to using@/throughout the codebase (update index.ts, all the re-exports, everything) or yeet this path alias entirely. No in-between nonsense.Likely an incorrect or invalid review comment.
tsconfig.json (1)
28-32:baseUrlisn't actually required anymore—stop spreading TS5090 FUD.TypeScript 4.1+ (released 2021) no longer requires
baseUrlto usepaths. The TS5090 error only hits you with pre-4.1 TypeScript, which is ancient at this point. That said, addingbaseUrl: "."is still solid practice for clarity and bundler compatibility—it signals intent and keeps module resolution explicit instead of relying on implicit behavior.If you're gonna use path aliases, just add it for good measure. Makes the config less magical.
Likely an incorrect or invalid review comment.
package.json (1)
31-38:@typescript/native-previewnightly: CLI and flags are solid, but you're living dangerously.The
tsgoCLI ships fine in this nightly build—--noEmitand-pboth work. The binary's there, it's documented, and it'll run your build/typecheck scripts without drama right now.But here's the thing—you're betting on a nightly staying stable. Nightlies drift. Hard to predict what breaks next week. Pinning the version (dropping the
^) is a clean move if you want to avoid your CI suddenly choking when you runnpm installfresh. It's not a blocker, but it's the kind of "pay 30 seconds now, save 3 hours of debugging Wednesday" move that separates boring, reliable builds from "why is CI red and nobody knows why" situations.If you're intentionally tracking the nightly for the performance gains, keep the
^and own the instability. If you want predictability, pin it. Either way, document the choice so the next person knows what they're signing up for.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
1 participant
Remove the example/ directory and .trae/ rules. Update CI workflow,
biome config, tsconfig, and package.json for fiber architecture.
Summary by CodeRabbit
New Features
Chores