Update modern-tooling plan: all phases complete#320
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 32 minutes and 53 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThe pull request updates the modern-tooling roadmap documentation by marking the knip implementation as merged, adding PR references and outcome notes, and expanding Phase 6 with new sub-items including Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plans/modern-tooling.md`:
- Around line 15-20: Update the "Phase 6 (future)" narrative so it matches the
progress table: rename or remove the "future" label and expand the Phase 6
writeup to list the sub-items verbatimModuleSyntax, ESM extensions,
strictEquality, and CI speed with their current statuses (6b:
verbatimModuleSyntax — PR open `#319`, 6c: ESM extensions — Merged `#315`, 6d:
strictEquality — Merged `#314`, 6e: CI speed — PR open `#318`), and adjust the prose
for each item to reflect their actual outcomes (e.g., note that
verbatimModuleSyntax aims to remove tslib, ESM extensions adds .js import fixes
and verify:esm CI, strictEquality implements
defineOption/ko.options.strictEquality, CI speed uses Python-based Bun install).
Ensure there are no contradictory "future" statements left in the Phase 6
narrative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Pull request overview
Updates the Modern Tooling Migration plan to reflect current progress and expand the list of future follow-ups.
Changes:
- Updates the progress table to mark knip work as merged and adds additional sub-phases (6b–6e).
- Expands “Future considerations” with additional potential follow-up items.
| | 6b. verbatimModuleSyntax | PR open | #319 | Enable verbatimModuleSyntax, remove tslib from all packages. | | ||
| | 6c. ESM extensions | Merged | #315 | Add .js extensions to ESM dist imports. verify:esm CI check. | | ||
| | 6d. strictEquality | Merged | #314 | defineOption API, ko.options.strictEquality setter. Fixes #290. | | ||
| | 6e. CI speed | PR open | #318 | Python-based Bun install in containers, saves ~9 min. | |
There was a problem hiding this comment.
The PR title says all phases are complete, but the progress table still lists phases 6b and 6e as "PR open". Also, later in this document Phase 6 is still labeled as "(future)" while this table now marks 6 as merged. Please reconcile the plan status messaging (either update the title/statuses or adjust the Phase sections so the table and narrative agree).
| - **Renovate/Dependabot** — automated dependency PRs (48h minimumReleaseAge guards supply chain) | ||
| - **Benchmarking** — vitest bench for observable/computed hot paths | ||
| - **`.github/copilot-instructions.md`** — extend AGENTS.md context to Copilot/Cursor users |
There was a problem hiding this comment.
These "Future considerations" bullets appear to describe work that already exists in the repo: .github/dependabot.yml is present, and .github/copilot-instructions.md already exists (and another plan even notes it was added). Consider updating/removing these items or rephrasing them as follow-ups (e.g., tuning Dependabot settings / minimumReleaseAge policy, or updating Copilot instructions). Also, the parenthetical "48h minimumReleaseAge guards supply chain" is a bit unclear—suggest rewording to explicitly say it guards against supply-chain attacks.
| - **Renovate/Dependabot** — automated dependency PRs (48h minimumReleaseAge guards supply chain) | |
| - **Benchmarking** — vitest bench for observable/computed hot paths | |
| - **`.github/copilot-instructions.md`** — extend AGENTS.md context to Copilot/Cursor users | |
| - **Dependabot tuning** — refine automated dependency PR settings, including whether the 48h `minimumReleaseAge` policy remains appropriate to help guard against supply-chain attacks | |
| - **Benchmarking** — vitest bench for observable/computed hot paths | |
| - **`.github/copilot-instructions.md` updates** — extend existing guidance with more AGENTS.md context for Copilot/Cursor users |
brianmhunt
force-pushed
the
docs/update-plans
branch
from
81f896a to
004dd02
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
plans/dark-factory.md: vision for AI-maintained repo — what's in place, what's missing, and the principles behind it. agents/soul.md: the philosophical foundation of Knockout — observables as the atom, update locality, provider architecture, backwards compat. Gives AI agents the *why* behind design decisions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add five-level framing, StrongDM's "code must not be written/reviewed by humans" approach, Stanford CodeX trust paper. Position TKO at Level 3-4 with honest assessment of gaps. Add scenario testing and incremental trust principle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Knockout apps written a decade ago are still in production. TKO is the path of least resistance forward. Aim for stability, not rigid backwards compatibility — breaking changes are acceptable when they improve correctness/security and the migration is clear. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
README: remove stale references (Make, lerna, Prettier, ESLint, Sauce Labs, CircleCI, Coveralls). Add current tooling, architecture summary, dark factory direction, and soul.md link. Keywords: add 5-6 searchable npm keywords per package — common (knockout, tko) plus category-specific tags. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- README quick start now defaults to @tko/build.reference - build.knockout positioned as migration path with link to /3to4/ - Phase 6 narrative expanded with actual sub-item statuses and PR refs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The .py extension triggered a separate CodeQL python job that always failed. The script still runs via python3 explicitly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
brianmhunt
force-pushed
the
docs/update-plans
branch
from
791f8ca to
4030dfc
Compare
Update progress table and future considerations.
Summary by CodeRabbit
Documentation