docs: add TypeScript Ultimate skill pack to Mandu#167
Conversation
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a comprehensive TypeScript Ultimate skill documentation suite to the docs directory, including guides on compiler configuration, type systems, API typing, framework-specific playbooks, data boundaries, testing strategies, and code review standards. Introduces a Python script to scrape and maintain a skills corpus index. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Summary of ChangesHello @RaptBliss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Mandu framework's documentation by introducing a complete 'TypeScript Ultimate' skill pack. This addition provides extensive guidance and best practices for developing robust TypeScript applications, covering everything from compiler settings to advanced type patterns and code review. The changes are purely documentation-focused, aiming to equip users with a deep understanding of high-quality TypeScript engineering within the Mandu ecosystem. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive TypeScript skill pack for the Mandu framework, including detailed workflows, mandatory rules, and framework-specific playbooks. While the refresh_corpus.py script is a valuable tool for maintaining the skill index, it unfortunately introduces a medium-severity Server-Side Request Forgery (SSRF) vulnerability due to insufficient validation of URLs parsed from external sources. Additionally, general feedback focuses on enhancing the script's robustness regarding network request reliability, error handling, and file encoding, along with a minor formatting fix for the documentation index.
| def get(url: str) -> str: | ||
| return urllib.request.urlopen(url, timeout=25).read().decode("utf-8", "ignore") | ||
|
|
||
|
|
||
| def extract_links(search_html: str): | ||
| # Extract visible skill links from list items (/owner/repo/skill) | ||
| links = sorted(set(re.findall(r'href="(/[^"#? ]+)"', search_html))) | ||
| return [l for l in links if len([p for p in l.split('/') if p]) == 3] | ||
|
|
||
|
|
||
| def main(): | ||
| OUT.mkdir(parents=True, exist_ok=True) | ||
| (OUT / "raw").mkdir(exist_ok=True) | ||
| (OUT / "text").mkdir(exist_ok=True) | ||
|
|
||
| html = get(QUERY) | ||
| links = extract_links(html) | ||
|
|
||
| index_lines = [] | ||
| for link in links: | ||
| url = BASE + link | ||
| slug = link.strip('/').replace('/', '__').replace(':', '-') | ||
| try: | ||
| page = get(url) |
There was a problem hiding this comment.
The refresh_corpus.py script is vulnerable to a Server-Side Request Forgery (SSRF). It fetches and parses a remote HTML page to extract links, then makes new requests to those links without proper validation. An attacker who can influence the content of the initial page (https://skills.sh/?q=Type) could cause the script to make requests to unintended endpoints on the skills.sh domain, potentially allowing them to use the script runner as a proxy for abuse or information gathering. Additionally, the default urllib User-Agent is often blocked by web servers. Consider adding a standard browser User-Agent header to urllib requests to make the script more resilient when fetching content from external sites.
| (OUT / "raw").mkdir(exist_ok=True) | ||
| (OUT / "text").mkdir(exist_ok=True) | ||
|
|
||
| html = get(QUERY) |
There was a problem hiding this comment.
The initial request to fetch search results is not protected by a try...except block. If the network is unavailable or the server returns an error, the script will crash. Handling this exception allows for a more graceful failure.
| html = get(QUERY) | |
| try: | |
| html = get(QUERY) | |
| except Exception as e: | |
| print(f"Failed to fetch search results from {QUERY}: {e}") | |
| return |
| page = get(url) | ||
| except Exception: | ||
| continue | ||
| (OUT / "raw" / f"{slug}.html").write_text(page) |
There was a problem hiding this comment.
Explicitly specifying encoding='utf-8' when writing files is a best practice to ensure consistent behavior across different operating systems where the default encoding might vary.
| (OUT / "raw" / f"{slug}.html").write_text(page) | |
| (OUT / "raw" / f"{slug}.html").write_text(page, encoding="utf-8") |
| (OUT / "text" / f"{slug}.txt").write_text(txt) | ||
| index_lines.append(f"{link}\t{len(txt)}") | ||
|
|
||
| (OUT / "index.tsv").write_text("\n".join(index_lines) + "\n") |
There was a problem hiding this comment.
Specifying the encoding here ensures the index file is written correctly regardless of the environment's default settings.
| (OUT / "index.tsv").write_text("\n".join(index_lines) + "\n") | |
| (OUT / "index.tsv").write_text("\n".join(index_lines) + "\n", encoding="utf-8") |
| 47. https://skills.sh/mindrally/skills/fastify-typescript | ||
| 48. https://skills.sh/sickn33/antigravity-awesome-skills/javascript-typescript-typescript-scaffold | ||
| 49. https://skills.sh/siviter-xyz/dot-agent/typescript | ||
| 50. https://skills.sh/asyrafhussin/agent-skills/typescript-react-patterns No newline at end of file |
There was a problem hiding this comment.
The file is missing a trailing newline. Adding one is a standard convention for text files and improves compatibility with various CLI tools.
| 50. https://skills.sh/asyrafhussin/agent-skills/typescript-react-patterns | |
| 50. https://skills.sh/asyrafhussin/agent-skills/typescript-react-patterns | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
docs/skills/typescript-ultimate/scripts/refresh_corpus.py (4)
23-23: Rename unusedattrsparameter to_attrsto suppress the lint warning.The
HTMLParserAPI requires the signature butattrsis never read here (Ruff ARG002).♻️ Proposed fix
- def handle_starttag(self, tag, attrs): + def handle_starttag(self, tag, _attrs):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py` at line 23, The handle_starttag method currently declares an unused parameter named attrs which triggers a lint ARG002; rename the parameter to _attrs in the def handle_starttag(self, tag, _attrs): signature so the HTMLParser API signature is preserved but the linter recognizes the parameter as intentionally unused; update any internal references if present (none expected) and run the linter to confirm the warning is suppressed.
55-55: Renamelto avoid the ambiguous variable name lint error (Ruff E741).
lis easily confused with1orI.♻️ Proposed fix
- return [l for l in links if len([p for p in l.split('/') if p]) == 3] + return [lnk for lnk in links if len([p for p in lnk.split('/') if p]) == 3]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py` at line 55, Return expression uses a single-letter variable `l`; rename it to a descriptive name (e.g., `link`) to fix the Ruff E741 lint error. Update the list comprehension in the function that returns filtered links so it reads something like `return [link for link in links if len([part for part in link.split('/') if part]) == 3]`, ensuring any other occurrences of `l` in that expression are similarly renamed and tests/uses of the function are updated to match.
70-73: Log the caught exception instead of silently swallowing it (Ruff S112 / BLE001).Silent
except Exception: continuemakes it impossible to diagnose fetch failures during a corpus refresh run. At minimum, print a warning to stderr.♻️ Proposed fix
+import sys ... try: page = get(url) - except Exception: + except Exception as exc: + print(f"Warning: skipping {url}: {exc}", file=sys.stderr) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py` around lines 70 - 73, The try/except that currently swallows all errors around the call to get(url) should log the caught exception (including the URL) before continuing; modify the block around page = get(url) to catch Exception as e and call logging.warning or print to stderr with a clear message like "Failed to fetch {url}: {e}" (ensure logging is imported and configured or use sys.stderr), then continue as before so failures are visible for debugging.
49-49: Use awithcontext manager forurlopento ensure the connection is released promptly.Calling
.read()immediately consumes the body, but the underlyingHTTPResponseobject is not closed until GC runs. In a loop fetching 50+ URLs this can leave connections open longer than necessary.♻️ Proposed fix
def get(url: str) -> str: - return urllib.request.urlopen(url, timeout=25).read().decode("utf-8", "ignore") + with urllib.request.urlopen(url, timeout=25) as resp: + return resp.read().decode("utf-8", "ignore")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py` at line 49, The call to urllib.request.urlopen(...) should be wrapped in a context manager so the HTTPResponse is closed promptly; replace the direct return of urllib.request.urlopen(url, timeout=25).read().decode(...) with using "with urllib.request.urlopen(url, timeout=25) as resp:" and then read and decode from resp (resp.read().decode("utf-8", "ignore")) so connections are released immediately; update the function in refresh_corpus.py where urllib.request.urlopen is used.docs/README.md (1)
13-13: LGTM!Clean, consistent addition. Optionally, the Document Status table (lines 69–90) could include an entry for
docs/skills/README.md(and the new skill files) to keep the log in sync with the rest of the tracked docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/README.md` at line 13, Add an entry for docs/skills/README.md (and each new skill file) to the "Document Status" table so the new docs are tracked consistently with the rest of the repository; locate the "Document Status" table in README.md and append rows for docs/skills/README.md and the new skill filenames with the same columns/format used by existing entries to match status, owner, and last-updated fields.docs/skills/typescript-ultimate/references/07-code-review-and-standards.md (1)
17-19: Optional: break up consecutive "Prefer" openers (linter hint).✏️ Suggested rewording
- Prefer simple, obvious code over clever abstractions. - Prefer early returns over deep nesting. -- Prefer functions over static-only classes. +- Favour functions over static-only classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/references/07-code-review-and-standards.md` around lines 17 - 19, The three consecutive bullets starting with "Prefer" ("Prefer simple, obvious code over clever abstractions.", "Prefer early returns over deep nesting.", "Prefer functions over static-only classes.") should be reworded to avoid repetitive openers; rewrite each line to vary the phrasing (e.g., "Favor simple, obvious code over clever abstractions.", "Use early returns instead of deep nesting.", "Choose functions rather than static-only classes.") so the guidance stays identical but the sentence openings are different and more readable.docs/skills/typescript-ultimate/references/06-testing-types-and-behavior.md (1)
12-14: Optional: name the compile-failure testing mechanism on line 14."Expect compile failure" is actionable only if readers know which tool to reach for. Common options are
//@ts-expect-error``,tsd, or the `expect-type` library. A brief parenthetical would make this bullet as concrete as the others.✏️ Suggested rewording
- - Negative tests for invalid assignments (expect compile failure). + - Negative tests for invalid assignments (use `@ts-expect-error` or `tsd`/`expect-type` to assert compile failures).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/references/06-testing-types-and-behavior.md` around lines 12 - 14, Update the third bullet "Negative tests for invalid assignments (expect compile failure)" to name a specific compile-failure mechanism so readers know what to use; for example change the parenthetical to "(e.g., // `@ts-expect-error`, the tsd test runner, or the expect-type library)" so the line reads "Negative tests for invalid assignments (e.g., // `@ts-expect-error`, the tsd test runner, or the expect-type library)". This targets the exact bullet text and gives concrete, commonly used options.docs/skills/typescript-ultimate/SKILL.md (1)
63-65: Optional: vary sentence openers in Execution Guidance for readability.Three consecutive bullets start with "For", flagged by the linter. A minor rewording removes the repetition.
✏️ Suggested rewording
- - For small fixes: apply minimal patch + enforce local invariants. - - For medium changes: establish/repair types first, then implementation. - - For large migrations: convert module-by-module, maintain compatibility adapters, and increase strictness in stages. + - Small fixes: apply minimal patch + enforce local invariants. + - Medium changes: establish/repair types first, then implementation. + - Large migrations: convert module-by-module, maintain compatibility adapters, and increase strictness in stages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skills/typescript-ultimate/SKILL.md` around lines 63 - 65, The three consecutive bullets beginning with "For small fixes: apply minimal patch + enforce local invariants.", "For medium changes: establish/repair types first, then implementation.", and "For large migrations: convert module-by-module, maintain compatibility adapters, and increase strictness in stages." should be reworded to avoid repeating the opener "For"; update each bullet to use varied sentence openers (e.g., "When applying small fixes:", "When handling medium changes:", "During large migrations:") or similar concise alternatives while preserving the existing guidance and punctuation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py`:
- Line 12: The search query in the refresh script uses QUERY = f"{BASE}/?q=Type"
which is too broad and pulls non-TypeScript skills; update QUERY to use
"?q=TypeScript" (e.g., QUERY = f"{BASE}/?q=TypeScript") so only
TypeScript-related entries are returned, then re-run the script to regenerate
source-corpus-index.md; make this change in the file where QUERY and BASE are
defined.
- Around line 74-88: The Path.write_text calls in the block (e.g., (OUT / "raw"
/ f"{slug}.html").write_text(page), (OUT / "text" /
f"{slug}.txt").write_text(txt), and (OUT / "index.tsv").write_text(...)) omit
encoding and will use the system locale; update each write_text invocation to
pass encoding="utf-8" to ensure written HTML, extracted text from MainText(),
and the index TSV are encoded as UTF-8 consistently across platforms.
---
Nitpick comments:
In `@docs/README.md`:
- Line 13: Add an entry for docs/skills/README.md (and each new skill file) to
the "Document Status" table so the new docs are tracked consistently with the
rest of the repository; locate the "Document Status" table in README.md and
append rows for docs/skills/README.md and the new skill filenames with the same
columns/format used by existing entries to match status, owner, and last-updated
fields.
In `@docs/skills/typescript-ultimate/references/06-testing-types-and-behavior.md`:
- Around line 12-14: Update the third bullet "Negative tests for invalid
assignments (expect compile failure)" to name a specific compile-failure
mechanism so readers know what to use; for example change the parenthetical to
"(e.g., // `@ts-expect-error`, the tsd test runner, or the expect-type library)"
so the line reads "Negative tests for invalid assignments (e.g., //
`@ts-expect-error`, the tsd test runner, or the expect-type library)". This
targets the exact bullet text and gives concrete, commonly used options.
In `@docs/skills/typescript-ultimate/references/07-code-review-and-standards.md`:
- Around line 17-19: The three consecutive bullets starting with "Prefer"
("Prefer simple, obvious code over clever abstractions.", "Prefer early returns
over deep nesting.", "Prefer functions over static-only classes.") should be
reworded to avoid repetitive openers; rewrite each line to vary the phrasing
(e.g., "Favor simple, obvious code over clever abstractions.", "Use early
returns instead of deep nesting.", "Choose functions rather than static-only
classes.") so the guidance stays identical but the sentence openings are
different and more readable.
In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py`:
- Line 23: The handle_starttag method currently declares an unused parameter
named attrs which triggers a lint ARG002; rename the parameter to _attrs in the
def handle_starttag(self, tag, _attrs): signature so the HTMLParser API
signature is preserved but the linter recognizes the parameter as intentionally
unused; update any internal references if present (none expected) and run the
linter to confirm the warning is suppressed.
- Line 55: Return expression uses a single-letter variable `l`; rename it to a
descriptive name (e.g., `link`) to fix the Ruff E741 lint error. Update the list
comprehension in the function that returns filtered links so it reads something
like `return [link for link in links if len([part for part in link.split('/') if
part]) == 3]`, ensuring any other occurrences of `l` in that expression are
similarly renamed and tests/uses of the function are updated to match.
- Around line 70-73: The try/except that currently swallows all errors around
the call to get(url) should log the caught exception (including the URL) before
continuing; modify the block around page = get(url) to catch Exception as e and
call logging.warning or print to stderr with a clear message like "Failed to
fetch {url}: {e}" (ensure logging is imported and configured or use sys.stderr),
then continue as before so failures are visible for debugging.
- Line 49: The call to urllib.request.urlopen(...) should be wrapped in a
context manager so the HTTPResponse is closed promptly; replace the direct
return of urllib.request.urlopen(url, timeout=25).read().decode(...) with using
"with urllib.request.urlopen(url, timeout=25) as resp:" and then read and decode
from resp (resp.read().decode("utf-8", "ignore")) so connections are released
immediately; update the function in refresh_corpus.py where
urllib.request.urlopen is used.
In `@docs/skills/typescript-ultimate/SKILL.md`:
- Around line 63-65: The three consecutive bullets beginning with "For small
fixes: apply minimal patch + enforce local invariants.", "For medium changes:
establish/repair types first, then implementation.", and "For large migrations:
convert module-by-module, maintain compatibility adapters, and increase
strictness in stages." should be reworded to avoid repeating the opener "For";
update each bullet to use varied sentence openers (e.g., "When applying small
fixes:", "When handling medium changes:", "During large migrations:") or similar
concise alternatives while preserving the existing guidance and punctuation.
| import urllib.request | ||
|
|
||
| BASE = "https://skills.sh" | ||
| QUERY = f"{BASE}/?q=Type" |
There was a problem hiding this comment.
Narrow the search query from ?q=Type to ?q=TypeScript to avoid off-topic corpus entries.
The current query matches any skill containing "Type", pulling in Python, Rust, and Symfony entries. The generated source-corpus-index.md reflects this — items 4 (python-type-safety), 15 (m05-type-driven from a Rust skills repo), 34 (prototype-prompt-generator), and 36 (symfony:form-types-validation) are not TypeScript-related. The corpus index should be regenerated after tightening the query.
🐛 Proposed fix
-QUERY = f"{BASE}/?q=Type"
+QUERY = f"{BASE}/?q=TypeScript"📝 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.
| QUERY = f"{BASE}/?q=Type" | |
| QUERY = f"{BASE}/?q=TypeScript" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py` at line 12, The
search query in the refresh script uses QUERY = f"{BASE}/?q=Type" which is too
broad and pulls non-TypeScript skills; update QUERY to use "?q=TypeScript"
(e.g., QUERY = f"{BASE}/?q=TypeScript") so only TypeScript-related entries are
returned, then re-run the script to regenerate source-corpus-index.md; make this
change in the file where QUERY and BASE are defined.
| (OUT / "raw" / f"{slug}.html").write_text(page) | ||
|
|
||
| p = MainText() | ||
| p.feed(page) | ||
| txt = p.text() | ||
| if len(txt) < 200: | ||
| txt = re.sub(r"<script[\s\S]*?</script>", " ", page) | ||
| txt = re.sub(r"<style[\s\S]*?</style>", " ", txt) | ||
| txt = re.sub(r"<[^>]+>", " ", txt) | ||
| txt = re.sub(r"\s+", " ", txt).strip() | ||
|
|
||
| (OUT / "text" / f"{slug}.txt").write_text(txt) | ||
| index_lines.append(f"{link}\t{len(txt)}") | ||
|
|
||
| (OUT / "index.tsv").write_text("\n".join(index_lines) + "\n") |
There was a problem hiding this comment.
Specify encoding="utf-8" on all write_text calls for cross-platform consistency.
Path.write_text defaults to the system locale encoding, which may not be UTF-8 on Windows. Since get() already decodes bytes as UTF-8, writing back without the same encoding can corrupt non-ASCII content on non-UTF-8 locales.
🐛 Proposed fix
- (OUT / "raw" / f"{slug}.html").write_text(page)
+ (OUT / "raw" / f"{slug}.html").write_text(page, encoding="utf-8")
...
- (OUT / "text" / f"{slug}.txt").write_text(txt)
+ (OUT / "text" / f"{slug}.txt").write_text(txt, encoding="utf-8")
...
- (OUT / "index.tsv").write_text("\n".join(index_lines) + "\n")
+ (OUT / "index.tsv").write_text("\n".join(index_lines) + "\n", encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/skills/typescript-ultimate/scripts/refresh_corpus.py` around lines 74 -
88, The Path.write_text calls in the block (e.g., (OUT / "raw" /
f"{slug}.html").write_text(page), (OUT / "text" /
f"{slug}.txt").write_text(txt), and (OUT / "index.tsv").write_text(...)) omit
encoding and will use the system locale; update each write_text invocation to
pass encoding="utf-8" to ensure written HTML, extracted text from MainText(),
and the index TSV are encoded as UTF-8 consistently across platforms.
konamgil
force-pushed
the
feat/typescript-ultimate-skill-pack
branch
from
f2776e2 to
2b6ba1f
Compare
1 participant
Summary
docs/skills/typescript-ultimatedocs/skills/README.mdand link skills section fromdocs/README.mdWhy
Scope
Summary by CodeRabbit