원클릭으로
code-review
// Review a GitHub PR or local diff for the @ilamy/calendar repo. Drafts findings as Conventional Comments and waits for explicit approval before posting anything to GitHub. Use when the user says "review this PR", "review PR
// Review a GitHub PR or local diff for the @ilamy/calendar repo. Drafts findings as Conventional Comments and waits for explicit approval before posting anything to GitHub. Use when the user says "review this PR", "review PR
| name | code-review |
| description | Review a GitHub PR or local diff for the @ilamy/calendar repo. Drafts findings as Conventional Comments and waits for explicit approval before posting anything to GitHub. Use when the user says "review this PR", "review PR |
Review a pull request or local diff thoroughly, draft findings as Conventional Comments anchored to specific lines, and present the draft to the user. Never post to GitHub without explicit approval.
These are non-negotiable. Violating any of these is a defect in the review.
blocking vs non-blocking vs if-minor) for the small subset where one is actually needed AND severity is genuinely ambiguous. Never silently add (blocking) yourself.REQUEST_CHANGES), the rule is: either the body adds unique structural context (workflow feedback, "this PR is stacked on #N"), or you skip the review wrapper entirely and post individual inline comments via /pulls/{N}/comments.useMemo in one file, grep the diff for other useMemo blocks and check each. If you call out a manual Intl-based helper, look for siblings. The user explicitly asked "look for other such options" when a single instance was flagged and other instances existed nearby.(blocking) and reference the prior review comment.min-w-*) without responsive view switching is almost always wrong on a calendar with 7+ columns; the failure case is the smallest viewport × largest column count.min-w-20 because fully fluid is the design"), that closes the intent question but not the correctness question. Always separately ask: given they intended this, does the resulting behavior actually work? For layout / observable-behavior comments, the default on a re-review is not resolved until you've either verified visually in the running dev server OR articulated concretely (numbers, walkthrough) why the change is safe. Author claim + dev log entry alone do not move a comment from open to resolved.gh api repos/<owner>/<repo>/pulls/<N>/comments (or gh pr view <N> --json reviews,comments) and read every reply that arrived since you last posted. Author replies change what counts as "open" vs "resolved"; project-owner replies are authoritative. Don't trust the diff alone to tell you where prior comments stand.Before reading the diff for content, check the structure. Two problems are common in this repo's PRs and need to be surfaced before drafting line-level comments:
Run gh pr list --author <author> --state open --json number,headRefName to see the author's other open PRs. If multiple PRs exist and the diff for the current PR has 30+ files, the branches are probably stacked on top of each other rather than branched off main. Confirm by checking commits:
gh pr view <N> --json commits -q '.commits | length'
gh pr view <N-1> --json commits -q '.commits | length'
If PR N includes most of PR N-1's commits, the branches are stacked. Stop and flag this to the user before drafting line-level reviews. A stacked branch's diff cannot be reviewed in isolation. Suggest one of: (a) ask author to rebase each branch off main, (b) post a per-PR one-liner explaining the problem, or (c) close and reopen.
Read the PR title against the diff. If the title says "fix X" but the diff also rewrites Y, deletes public-API surface area, or introduces a new abstraction unrelated to X, that is out-of-scope. Flag it. The right move is to ask the author to split the PR, not to silently review a scope-creep PR as if the title were honest.
Examples of out-of-scope you've seen in this repo:
Intl.DateTimeFormat.When you find this, your draft becomes: "this PR is doing 4 things; the original problem is fixed by commits A and B; commits C-D should be split into PR #2; commit E into PR #3."
Determine the review target from the user's request:
gh pr view <N> --json title,body,state,author,files,commits,headRefOid plus gh pr diff <N>. Save headRefOid (you need it later for inline comments).git diff or git diff main...HEAD.git show <sha>.If the diff is large (>35KB), save to a file and read it with Read using offset/limit. Don't skim. Actually read the changed lines.
Read the surrounding context of any non-trivial change. A diff hunk alone hides what was removed or why a field exists. Open the full file when the hunk touches shared components, context, or public APIs.
For PRs, also read the linked issue (gh issue view <N>). Issues often contain constraints ("no public-API changes", "must work for X") that the PR may have silently violated.
For diffs >300 lines or >5 files, spawn three Explore agents in a single message. For smaller diffs, do this inline. Each agent gets the full diff so they work from the same source.
timezone prop. Intl.DateTimeFormat without a timeZone option formats in the system timezone, which is wrong for this calendar. The previous fix is documented in docs/logs/2026-03-29.md style entries.docs/logs/) describe a fix that this PR re-introduces. Don't trust the PR author's description; check the logs.DRY is the guiding principle here. Actively hunt for repetition: any time the same shape of code (same boilerplate in tests, same condition, same JSX wrapper, same setup block) appears 2-3+ times, propose extracting it. Repetition is the single most common review finding and the easiest to undervalue. Look for it everywhere: production code, tests, demo code, documentation snippets. Test files in particular accumulate repetitive setup that begs for helpers like renderXyzView, buildEvent, at(hour, minute).
Specific patterns to flag:
render(<Provider>...<X /></Provider>) boilerplate with one varying prop. Extract a renderX(overrides?) helper.locale = currentLocale || currentDate.locale()-style patterns across multiple files. Extract to a small hook so future changes (like adding timeZone) are one-line.cn() is the codebase convention.formatLocaleDate.ts) when a one-line plugin call would do (dayjs.extend(localizedFormat) + format('LL')), call it out with the comparison. Verify both produce identical output before claiming equivalence. For every new helper function in the diff, ask: "what does this do that the underlying library / plugin / standard token does not already do?" If the answer is "nothing meaningful" or "a stylistic preference", recommend deleting it.useMemo blocks. Two common shapes to flag:
const.[currentDate] where currentDate updates on every navigation, or [firstDayOfWeek] where the body is 7 cheap dayjs ops). The cache never hits, the hook bookkeeping costs more than the work. Drop the memo, inline the expression.useMemo, the comment must include a brief reason: "empty deps", "deps change every render", "7 cheap ops, no benefit", etc. Do not just say "drop the useMemo" without explaining why.The goal is not zealous deduplication. Three similar lines is sometimes better than a premature abstraction. But when the same shape appears with no real variance, name it once.
.agents/rules/code-style.md and .agents/rules/coding-patterns.md):
.at(0) / .at(-1) over [0] / [length - 1] for first/last array access.! non-null assertions. Use guards, optional chaining, or default values.Boolean(x) over !!x for coercion.any. Use unknown with type guards, specific interfaces, or generics.export default. Named exports only.cn(...) with multi-clause conditions, extract each conditional class to a named const, don't inline.foo.tsx + foo.test.tsx). Hooks and utils live with the feature that owns them. Shared code sits at the lowest common ancestor in the tree. A PR that moves a feature-specific util into a global lib/ should be questioned.a && b && c), extract to a named const that describes what the combined condition represents. Example: const canShowFeature = featureAvailable && featureEnabled && !userOptedOut then if (canShowFeature). Flag inline multi-clause conditions in if, && JSX gates, ternary tests, and cn(...) calls.isActive ? 'on' : 'off'). Flag a ternary if either (a) it nests another ternary in either branch (a ? b : c ? d : e), or (b) it wraps across multiple lines (the formatter broke cond ? long : alsoLong onto 3+ lines). Both mean the expression is too complex to read as a ternary. Suggest converting to if/else if/else, an early-return guard, a small named helper function, or a key-value lookup object. A multi-line ternary is a defect even when it is only one level deep.if or switch blocks. Suggest flattening with early returns, guard variables, or key-value object lookups. Nested control flow is almost always a sign that the logic can be expressed more clearly.{condition && <X />}) or early returns from the component. {isLoading ? <Spinner /> : <Content />} should become an early return for the loading branch or two consecutive && gates.useSmartCalendarContext, useEffectiveBusinessHours), it should read it there directly instead of accepting it as a prop or argument and having every caller plumb it through. Flag any case where a caller pulls a value out of context only to pass it back into a callee that could pull it itself. Same rule for refs, settings, callbacks, and any other context-derived data. Tests can wrap the callee in a minimal provider.useMemo / loops that run in modes where the result is unused. Guard with if (!isActive) return [].useMemo deps that breaks child memoization.useCallback and invoke at each site.useMemo(() => flag ? A() : B(), [...allDeps]) recomputes on every change. Split into two memos with narrower deps + a tiny selector.gridType === 'hour'), there should be a test that the feature mounts under the condition and not otherwise. Component-level tests in isolation don't catch a regressed gate.day-header.tsx inside resource-calendar/components/week-view/horizontal/ is wrong (too generic without path context). Use resource-week-horizontal-day-header.tsx. Names appear path-less in IDE tabs, grep, imports, stack traces.When agents return, filter findings:
Skip false positives silently. Don't pad the review with weak observations. A 2-comment review is better than an 8-comment review where 6 are fluff.
Follow the Conventional Comments format strictly.
Format:
<label> [decoration]: <one-line subject>
<optional body, 2-3 sentences default>
A blank line between subject and body is mandatory. GitHub renders the entire JSON body field as markdown; a missing blank line collapses everything into one paragraph and visually merges the subject with the body. When you write the JSON, that means two \n characters: "body": "praise: Right fix.\n\nuseRef(locale) matched the prop...". The single-\n form "praise: Right fix. useRef(locale) matched..." is broken even when it looks correct in your draft preview, because the chat preview folds whitespace.
Subject ends with a period. No em dashes anywhere.
Right:
"body": "praise: Right fix.\n\nuseRef(locale) matched the prop immediately on mount, so the effect skipped and dayjs.locale() never ran. useRef(undefined) forces the first run."
Wrong (renders as one paragraph, subject merges into body):
"body": "praise: Right fix. useRef(locale) matched the prop immediately on mount, so the effect skipped and dayjs.locale() never ran."
This is the failure mode you've shipped most recently. The subject and body looked separated in your draft preview but the JSON had a single space, not \n\n. Always check the JSON literally.
Labels (pick the one that matches):
praise: positive highlights. Only use if it's a specific, non-generic observation worth noting. Skip generic praise.nitpick: trivial preference. Inherently non-blocking, so the decoration is usually redundant.suggestion: proposed improvement with a clear reason.issue: a specific defect that needs attention.todo: small required change.question: you're uncertain and need clarification.thought: a reflection that may or may not need action.chore: required tasks before merging.note: informational only.Decorations are optional. Most comments do not need one. The default is no decoration.
(blocking): should prevent merge until resolved.(non-blocking): should not prevent merge.(if-minor): resolve only if the fix is trivial.Decide for yourself whether each comment needs a decoration at all, using this rubric:
nitpick, praise, note, question, thought: no decoration. The label already conveys severity. Don't add one. Don't ask.suggestion, issue, todo, chore: usually no decoration either. Add a decoration only when the comment's severity is materially different from what the label alone suggests, and a reader could plausibly misread it. Example: a suggestion that you believe must be addressed before merge becomes (blocking). A suggestion that is fine to defer needs no decoration. Use (if-minor) when the fix is worth doing only if it's cheap.Only ask the user about the choice of decoration when (a) a decoration is needed and (b) you genuinely cannot tell which one fits. Skip the ask when the user already signaled severity in conversation. If asking for multiple comments, batch them into a single AskUserQuestion call with one question per comment.
Body length: default to 2-3 sentences. Longer is fine when the context demands it (e.g., explaining why a refactor breaks consumers, or citing a previous fix's log entry). Shorter is fine when the subject is self-explanatory.
Inline review comments are the default in this repo, not top-level PR comments. Use the full file path and line number (or line range for multi-line context). The format the user sees:
**Inline N** at `<full file path>:<line>` (or `<start>-<end>` for ranges)
<conventional comment body>
Default to none. The body must add information that no inline comment carries. Two valid use cases:
REQUEST_CHANGES and COMMENT review events both require a non-empty body. If you have no unique structural content, do not use a review wrapper at all. Post individual inline comments via /pulls/{N}/comments (Phase 5, Strategy 1).Things that are not valid top-level bodies, even though they look summary-shaped:
(blocking) decoration.)If you cannot point at a sentence that says something no inline says, drop the body.
For each comment, decide whether a decoration is needed using the rubric in the previous section. The default is no decoration. Add (blocking), (non-blocking), or (if-minor) only when the comment's true severity is materially different from what the label alone implies.
If after applying the rubric you still cannot tell which decoration fits a comment, ask the user. Batch any asks into a single AskUserQuestion call (one question per still-ambiguous comment, up to four). Skip the ask entirely if every comment's decoration is already decided.
Present the decorated comments in order. End with: "Say 'post it' to submit as a single review on commit <sha>. Or tell me what to tweak."
Before showing the final decorated draft to the user, run through this checklist. Use TaskCreate to add one task per item and TaskUpdate to mark it completed only after verification. Do not skip items. Do not batch-mark. If an item cannot be checked off honestly, the draft is not ready.
gh api repos/<owner>/<repo>/pulls/<N>/comments and read every reply (author and project owner) that arrived since the last posted review. Each prior comment's status (open / resolved / contested) reflects the most recent message in its thread, not just the diff state.docs/logs/) checked for previously-fixed bugs that the PR might re-introduce.<label>: <subject> on one line, blank line, then optional body. Decorations added in a separate step (see below).\n\n between the subject line and the body in the JSON. Grep the review JSON for ": " followed by <label>: patterns and confirm the next characters before the body are \n\n, not a single space. A missing blank line renders the whole comment as one paragraph on GitHub and the subject visually merges with the body. Verified by opening the posted review URL after submission and confirming the subject sits on its own line.grep "—" on the draft returns nothing.suggestion includes a brief reason. "Drop this useMemo" is not enough; "Drop this useMemo. Empty deps and no inputs from component scope means it's a module constant in disguise." is.(blocking) with a link to the prior comment.min-w-* / min-h-* floor without responsive view switching is the failure pattern to look for. Numbers are surfaced in the comment so the author can see the cost.AskUserQuestion call. Never silently self-decide (blocking).headRefOid ready to pass as commit_id in the API call.After explicit user approval, choose the posting strategy based on whether any comment is (blocking) and whether you have a real structural observation for a top-level body.
The repo enforces this with a PreToolUse hook on Bash (.claude/hooks/check-pr-post-approval.sh). Any gh command that posts public-facing content (gh api .../pulls/.../{comments,reviews} -X POST, gh pr {comment,review,create}, gh issue {comment,create}) is blocked unless the same Bash command contains the literal substring touch .claude/state/pr-post-approved.flag (in the same chain, before the gh part).
The hook exists because the recurring failure mode is interpreting stale or implied approval as fresh. PreToolUse hooks fire BEFORE the Bash command runs, so a file-mtime check can't see the chained touch. Instead the hook scans the command text for the ritual, which makes approval an explicit, deliberate, auditable thing typed inline.
Rules:
touch and the gh post in a single Bash command so the touch is part of the same auditable invocation:
touch .claude/state/pr-post-approved.flag && gh api repos/<owner>/<repo>/pulls/<N>/comments -X POST --input /tmp/pr<N>-cmt-1.json
touch. The hook does not remember state between calls; that is intentional.The key question is "does my top-level body say something the inlines do not already say?" That decides the strategy, not the blocking status.
(All commands below must be chained with touch .claude/state/pr-post-approved.flag && to pass the hook. Examples in the Strategy sections show this.)
No body content beyond what's in the inlines. Post each comment as an individual inline review comment via POST /repos/{owner}/{repo}/pulls/{N}/comments. No review wrapper, no top-level body, no clutter. Works whether or not any comment is (blocking). The GitHub UI loses the "Changes requested" status flag, but each (blocking) decoration in the comment body still communicates severity to the author.
Real structural observation + blocking comments. Single REQUEST_CHANGES review via POST /repos/{owner}/{repo}/pulls/{N}/reviews. Top-level body is the structural observation (workflow feedback, prior-review reference, stacked-branch note). Do not summarize the inlines.
Real structural observation + no blocking comments. Single COMMENT review. Top-level body is the structural observation only.
If you haven't fetched the docs this session, WebFetch https://docs.github.com/en/rest/pulls/reviews and confirm the request body. The verified shapes:
Strategy 1 (individual inline comments), per comment:
{
"commit_id": "<headRefOid>",
"path": "<relative file path>",
"line": <int>,
"side": "RIGHT",
"body": "<conventional comment body>"
}
For multi-line range, replace line with start_line + line + start_side + side.
Post each via:
touch .claude/state/pr-post-approved.flag && gh api repos/<owner>/<repo>/pulls/<N>/comments -X POST --input /tmp/pr<N>-cmt-<i>.json
Strategy 2 or 3 (review with comments):
{
"commit_id": "<headRefOid>",
"event": "REQUEST_CHANGES" | "COMMENT",
"body": "<structural observation, never filler>",
"comments": [
{ "path": "...", "line": 81, "side": "RIGHT", "body": "..." },
{ "path": "...", "start_line": 45, "start_side": "RIGHT", "line": 50, "side": "RIGHT", "body": "..." }
]
}
Post via:
touch .claude/state/pr-post-approved.flag && gh api repos/<owner>/<repo>/pulls/<N>/reviews -X POST --input /tmp/pr<N>-review.json
Return the html_url from the response.
gh pr comment <N> --body-file <file> per PR. Acceptable as a top-level comment because there is no specific line to anchor to.gh issue comment <N> --body-file <file>.kcsujeet/ilamy-calendar (default).main. PRs to main. Stacked branches are a recurring anti-pattern..claude/hooks/check-dev-log.sh blocks session exit when src/ mtime is newer than today's docs/logs/YYYY-MM-DD.md. git checkout between branches updates mtimes as a side effect. If the hook fires after a review-only session, touch docs/logs/<today>.md (creating it with a brief "review-only" entry if absent) is enough.bun test. CI is bun run ci.For PR #122 (closes #121, adds now-line to horizontal grids):
**Inline 1** at `src/features/calendar/types/index.ts:81`
suggestion: Make `axis` optional rather than required.
Issue #121 explicitly states "No public-API changes." Optional gives the same UX (the library always passes the value), aligns with the issue's intent, and avoids a hard compile break for TS strict-mode consumers who construct the props as a literal. The component already defaults to `'vertical'` at `current-time-indicator.tsx:36`.
**Inline 2** at `src/components/horizontal-grid/horizontal-grid-events-layer.tsx:45-50`
issue: No integration test locks in the `gridType === 'hour'` gate.
Component-level tests verify axis behavior in isolation, but nothing asserts the indicator mounts here when `gridType === 'hour'` and not when `gridType === 'day'`. One render with `data-testid='current-time-indicator'` assertions per gridType would catch a future regression.
Apply the rubric: most comments stay un-decorated. For this PR, the public-API change in #1 may be (blocking) if the issue's "no public-API changes" constraint is authoritative; the missing test in #2 is (blocking) because untested gates regress silently. If those calls feel uncertain, batch them into one AskUserQuestion. Otherwise pick directly, show the final, and post.
Cut a new release of @ilamy/calendar — analyze commits since the last tag, suggest a semver bump, draft a CHANGELOG entry in the project's existing style, run the CI gate, commit, tag, push to origin, and create the GitHub release page (marked as `latest`). Pauses for the user to run `npm publish` interactively, then (on their signal) comments on every issue closed by the release and closes any still open. Use whenever the user says "release a new version", "cut a release", "ship v1.x", "bump the version", "prep a release", "publish the next version", "I just published X", "close the released issues", or anything that implies a new version should go out or a shipped version needs its issue follow-ups.
Systematically reduce the shipped bundle size of a JS/TS library without sacrificing code readability or breaking consumer APIs. Use this skill whenever the user mentions bundle size, tree-shaking, code duplication to cut, "reduce size of", "make lighter", "shrink output", looks for wins in `dist/`, or asks to audit a library's heaviest files. Also trigger when the user pastes a `bun run build` / `npm run build` / `bunup` / `tsup` / `rollup` output showing `dist/index.js` size and asks for reductions. Prefer this over vague "refactor to be smaller" answers — it enforces a measure-first loop, concrete opportunity categories, and readability guardrails.