Add TypeScript code review skill and updated findings plans#350
Add TypeScript code review skill and updated findings plans#350phillipc wants to merge 12 commits intoknockout:mainfrom
Conversation
- Created a TypeScript code review skill document outlining the review process, core categories, and output format. - Introduced two comprehensive plan for addressing TypeScript code review findings in the TKO monorepo. - Updated tsconfig.json to exclude new directories for skills and plans.
… round 1, and add new findings for rounds 3 and 4
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (1)
📝 WalkthroughWalkthroughAdded an "AI Skills" section to ChangesAI Skills & References
Plans / Findings (Rounds 1–5)
Project Config
Sequence Diagram(s)(omitted — documentation-only changes, no new multi-component runtime control flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 0/1 reviews remaining, refill in 55 minutes and 9 seconds.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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/typescript-code-review/references/tko-conventions.md`:
- Line 154: Update the package layout line that currently reads "Each package:
`src/`, `spec/`, `types/`, `dist/`, `index.ts`, `Makefile`" to the current
convention: list the required entries as `src/`, `spec/`, `dist/`, `helpers/`,
`index.ts`, and `package.json`. Replace the stale `types/` and `Makefile` items
with `helpers/` and `package.json` so the document reflects the real package
structure.
- Around line 60-66: The README/table entry for the TypeScript compilation
target is wrong: locate the table row containing the `target` setting (the row
that reads | `target` | `ES2020` | ...) and change the value cell from `ES2020`
to `ES2022` so the documentation matches tsconfig.json's `"target": "es2022"`.
- Around line 162-164: Update the testing conventions text to match the actual
setup: replace the "Mocha + Chai + Sinon" reference with "Vitest + Playwright
(browser mode)" and clarify that Playwright runs in browser mode by default and
is headless by default; also update the "Test location" entry to include both
packages/*/spec/ and builds/*/spec/**/*.js (to match vitest.config.ts). Ensure
the runner line explicitly says "Runner: Vitest + Playwright (browser mode,
headless by default)" and the test location line lists the two glob patterns so
it matches the configuration in vitest.config.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93a3cfae-f85b-494d-9f65-58d1502036cb
📒 Files selected for processing (9)
AGENTS.mdplans/typescript-code-review-findings-2.mdplans/typescript-code-review-findings-3.mdplans/typescript-code-review-findings-4.mdplans/typescript-code-review-findings.mdskills/typescript-code-review/SKILL.mdskills/typescript-code-review/references/review-reference.mdskills/typescript-code-review/references/tko-conventions.mdtsconfig.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
skills/typescript-code-review/references/tko-conventions.md (1)
161-161:⚠️ Potential issue | 🟡 MinorClarify the test runner mode and default browser behavior.
Line 161 is still underspecified. It should explicitly say Vitest runs in browser mode via Playwright and that the default is headless Chromium, to align reviewer expectations.
Based on learnings: “Applies to vitest.config.ts : Use Vitest as the test runner with browser mode via Playwright and headless Chromium”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/typescript-code-review/references/tko-conventions.md` at line 161, Update the line that currently reads "**Runner**: Vitest + Playwright" to explicitly state that Vitest should run in browser mode via Playwright and that the default browser is headless Chromium; mention the applicable config reference "vitest.config.ts" so readers know where to set test runner mode (browser) and the Playwright browser to headless Chromium.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@skills/typescript-code-review/references/tko-conventions.md`:
- Line 161: Update the line that currently reads "**Runner**: Vitest +
Playwright" to explicitly state that Vitest should run in browser mode via
Playwright and that the default browser is headless Chromium; mention the
applicable config reference "vitest.config.ts" so readers know where to set test
runner mode (browser) and the Playwright browser to headless Chromium.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1435a0c3-dd6b-4d9f-8600-e15bed63173d
📒 Files selected for processing (3)
AGENTS.mdskills/typescript-code-review/SKILL.mdskills/typescript-code-review/references/tko-conventions.md
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- skills/typescript-code-review/SKILL.md
…ferences and fix statuses
| - **Impact**: A custom `options.Promise`, cross-window promises, or thenables can be ignored or wrapped by the wrong | ||
| constructor. The existing "returns a promise" test only proves the default case where `options.Promise === Promise`. | ||
| - **Recommended**: Use a shared thenable check (for example `isThenable`) for completion detection and route framework | ||
| promise creation through `options.Promise`. Alternativ, you |
There was a problem hiding this comment.
@brianmhunt Want do you thing about options.Promise? Should we use oder remove it?
1 participant
Summary
Adds the TypeScript code review skill and four rounds of code review findings plans, updated to reflect the current TKO toolchain and review feedback.
Supersedes: #297 (closed draft)
What changed since #297
deletePropertytrap — fixed in Fix ko.proxy deleteProperty trap dropping the property key #336??earlyOut identical to||— confirmed, PR Fix ?? (nullish coalescing) earlyOut to not short-circuit on falsy values #345 openTextInputLegacyFirefoxdead code — confirmed, PR Remove dead TextInputLegacyFirefox class #346 openstylebinding jQuery reference — PR Fix jQuery reference in style update function #339 (merged)??parser findingdeletePropertyfix (Fix ko.proxy deleteProperty trap dropping the property key #336) as validation for the relatedgetOwnPropertyDescriptorfindingskills/typescript-code-review/) andtsconfig.jsonwiring includedFiles changed
skills/typescript-code-review/SKILL.md— code review skill definitionskills/typescript-code-review/references/tko-conventions.md— TKO-specific conventionsplans/typescript-code-review-findings.md— Round 1 (archived)plans/typescript-code-review-findings-2.md— Round 2 (active backlog + verification status)plans/typescript-code-review-findings-3.md— Round 3 (active backlog)plans/typescript-code-review-findings-4.md— Round 4 (active backlog)tsconfig.json— excludeskills/directoryVerification
bun run tsc— no type errors introduced (plans/skills are .md files)bun run check— lint/format cleanmainbranchSummary by CodeRabbit
Documentation
Chores