The senior principle (per Google eng-practices): reviewers should favor approving a Change List once it definitely improves the overall code health of the system, even if it isn't perfect. The bar isn't "perfect", it's is the codebase better with this merged than without it? Block only on real harm; raise style and polish as non-blocking notes.
Reviewer Checklist
- Design. Does the change fit the existing system or fight it?
- Functionality. Does it do what it says, including edge cases (empty / large input, concurrency, errors)?
- Complexity. Is it more complex than it needs to be? Watch for over-engineering.
- Tests. Do tests cover the new logic, not just happy paths?
- Naming & Comments. Clear names; comments explain why, not what.
- Style & Docs. Conforms to the style guide; public-facing changes update docs.
Goal: Find three open PRs and leave substantive comments on each, using
Conventional Comments labels. Substantive means
design or correctness, not formatting.
Reviews, posted as Ti-03, all non-blocking (COMMENTED)
1. excalidraw/excalidraw#11510, minimal fix for invalid hex-color feedback (issue #9527).
- suggestion: error text is surfaced only via the
title attribute, hover-only, inconsistent for screen readers, and the red border is a color-only cue (WCAG 1.4.1). Render a visible message and link it with aria-describedby.
- nit (non-blocking):
invalidColorTitle wraps a single-boolean ternary in useMemo, needless ceremony for a trivial t() lookup.
- praise: nice empty-input handling (
value.trim().length > 0) and clearing the state on blur.
2. excalidraw/excalidraw#11514, the thorough competing fix for the same issue #9527.
- suggestion: the message
"Use a format like #RRGGBB" is narrower than what normalizeInputColor actually accepts, short hex (#fff), named colors (red), and rgb()/hsl() all pass through tinycolor.
- suggestion: the
value === "" branch (empty = valid) is new logic with no test covering it.
- praise: correct accessibility pattern (
aria-invalid + aria-describedby → visible role="alert") and tests that go beyond the happy path.
3. calcom/cal.com#29603, enforce booking limits across all occurrences of a recurring series.
- question:
_checkBookingLimitsForRecurringBooking near-duplicates _checkBookingLimits (verbatim catch), fold the period-grouping + offset into the existing method instead?
- question: for a recurring reschedule, can a request that doesn't change the occurrence count be over-counted against the limit (since the offset counts every requested date)?
- praise: atomic up-front validation; the test asserting
bookingCount === 0 targets exactly the PENDING-bypass case.
Verification & Smoke Tests
verify-before-asserting
# For every claim: cloned the repo, read the real source, ran an isolated smoke test BEFORE posting.
git clone --depth 1 excalidraw && gh pr checkout 11510
node -e 'tinycolor("red").isValid()' # => true: normalizeInputColor DOES accept color names
# Two tempting Cal.com "bugs", both verified FALSE, so dropped:
401 status "regression" -> pre-existing; the regular method already wraps to 401
dropped includeManagedEvents -> regular path omits it too (consistent, not a gap)
# Per Google eng-practices, pre-existing patterns belong in a separate issue, not this CL.
Reflection
The most valuable part of this task wasn't writing comments, it was refusing to post any claim I hadn't verified against the real code. Reading a diff is enough to suspect something; it is not enough to assert it. Cloning each repo and running a tiny smoke test killed three comments that looked right on the diff but were wrong in reality, including a Cal.com "status regression" and an Excalidraw "does it even accept color names?" that tinycolor immediately disproved. That maps directly onto the week's principle: ask before stating. Where I genuinely didn't know (the recurring-reschedule offset), I asked a question instead of asserting a bug, and where the issue was real but pre-existing, I left it out of the CL rather than blocking on it. Pairing each review with honest praise made the critical notes land as collaboration, not attack.
Goal: Take one of my own past PRs, the one I'm least proud of. Pretend I'm the reviewer and write what I would have said.
PR Under Review
Ti-03/remainders#30, "Dev" · +4,461 / −105 across 33 files, merged. A whole dev branch merged into main under a one-word title, the honest "least proud of" choice: it breaks nearly every rule this week teaches.
The Review I'd Leave Now (as a hostile stranger)
- blocking (correctness): the Ko-fi webhook lowercases
donorEmail and queries where('email','==', donorEmail), but signup stores user.email un-normalized (dashboard:370 → firebase.ts:239). Firestore equality is case-sensitive, so an already-registered user with any uppercase in their email silently misses their paid Pro grant (filed pending_signup; auto-apply only fires at username creation). Fix: lowercase email at signup, as username already is.
- blocking (scope): 4,461 lines titled "Dev" bundling ~5 unrelated features, admin route-protection, a users page, a payment webhook, background uploads, and wallpaper caching. Unreviewable as one unit; the payment path alone deserved its own PR.
- suggestion (security): no replay/idempotency guard,
kofi_transaction_id exists for dedup but is never checked, so a retried or replayed valid payload re-runs the grant and resets planExpiresAt to now+30d.
- question: each payment overwrites
planExpiresAt to now+30d rather than extending it, a subscriber paying before expiry loses remaining days. Intended?
- praise: the webhook security basics are genuinely solid, timing-safe token compare with a length guard, fail-closed (503) when unconfigured, every event logged, and tolerant of Ko-fi's content-type quirks.
Verification, even on my own code
trace-before-assert
# Every claim traced to specific lines before it went in this report.
dropped: "webhook has no verification" # false, it does a timing-safe token compare
corrected: "user never gets Pro" # overstated, donate-before-signup + manual
# fallback recover it; bug hits already-registered
# users with mixed-case emails
confirmed: email-case bug · scope · replay gap · expiry overwrite
Reflection
Reviewing my own PR as a stranger caught a real payment bug I shipped and never noticed, and the harder discipline was being honest about severity. My first instinct was "users never get the Pro they paid for"; tracing applyPendingKofiGrant showed that's only true for one sub-case, so I narrowed the claim instead of keeping the scarier version. Self-review before requesting review really does catch a chunk of issues, but only if you read the code as adversarially as you'd read a stranger's, and resist the urge to either defend it or over-dramatize it. The "Dev" title and 4,000-line scope are their own lesson: I made that PR un-reviewable, which is a failure of my craft, not the reviewer's patience.
Goal: Find a real PR (open or merged) in a major project showing over-engineering or premature abstraction. Analyze: what's premature, the simpler alternative, and what would make the abstraction actually justified. The discipline is YAGNI, generalize when you have at least two real callers, not before.
PR Analyzed
microsoft/vscode#298676, "Introduce FoldingPreferences foundation and 'includeClosures' compatibility layer" · +829 / −242 across 9 files, open.
In one sentence: someone wanted code folding to optionally include the closing } line, and the PR builds an entire generic preferences framework to deliver that one behavior.
What's Premature (verified, not opinion)
CompatibilityAdjuster<P>, an abstract generic base class with exactly one subclass, CompatibilityAdjusterIncludeClosures. The code even comments "if multiple adjusters are active…", but there is only one.FoldingPreferencesCapabilities, an interface for providers to declare native support, yet every provider ships an empty {} (both IndentRangeProvider and SyntaxRangeProvider). The fancy interface is unused.- A new public
editor.foldingPreferences API that leaks into monaco.d.ts + standaloneEnums.ts, 829 lines of surface for a single preference.
- The author's own words: "only partially implemented and represented by placeholders."
The Maintainer's Pushback = the Analysis
aeschli (owns folding on the VS Code team): "There's no language-agnostic way of describing what you want. Each language is different. Such a general setting will just cause more confusion than benefits… It doesn't help if only a few providers support it… the right answer is specific settings per folding provider."
Simpler Alternative & the Justification Test
- Simpler: add
includeClosures as a setting on the one folding provider that needs it (e.g. the TS folding strategy), no foundation, no capabilities interface, no compatibility/adjuster layer, no public-API change. A handful of lines instead of 829.
- What would justify the abstraction: ≥2 real preferences that genuinely share a language-agnostic meaning and multiple providers natively supporting them. With 1 preference, 1 adjuster, 0 native supporters, the "two real callers" (YAGNI) test fails → premature.
Reflection
The test for premature abstraction isn't "is this clean?", this PR is tidy and well-documented, it's "where's the second real caller?" With one preference, one adjuster, and every provider declaring zero native capabilities, the whole framework is built for a future that hasn't arrived. The most useful realization was that the verdict is checkable, not a matter of taste: I confirmed the single subclass, the empty {} capabilities, and the public-API change directly in the diff, and the folding owner independently reached the same conclusion ("specific settings per provider"). YAGNI in one line: generalize when you have two real callers, not before.
Goal: Pick a project with a strong review culture (Rust, Kubernetes, VS Code, Django). Read the discussion on multiple merged PRs and summarize their review norms, what gets flagged, what doesn't, and how disagreements get resolved.
Structure & Gatekeeping
- Approval is a command, not a vibe.
@bors r+ approves and queues; r=me, r+ rollup for batching. Nothing merges except through the bors merge queue (the "not rocket science rule", main is always green).
- Assignment + state are explicit.
rustbot auto-assigns a reviewer ("within two weeks or reassign"); r? compiler picks a team. State is tracked by labels/commands: @rustbot author, @rustbot ready, S-waiting-on-perf.
- Rollups batch small approved PRs into one CI run for throughput.
What Gets Flagged vs. What Slides
- Flagged (perf): measured, not argued.
@bors try @rust-timer queue runs a benchmark before approval (#158042, #158122).
- Flagged (commit hygiene): a bot nags to move issue links out of commit messages (avoid spamming issues).
- Flagged (unverified claims & flaky CI): a "couldn't reproduce" was challenged and tested; reviewers triage "the CI failure is spurious."
- Slides: formatting/style is delegated to
tidy + bots, no human bikeshedding. PRs are approved when they clearly improve code health even with open follow-up questions ("glad to see this ugly workaround removed").
How Disagreements Resolve (#158137)
A reviewer doubted the author's "I couldn't reproduce the bug" claim. Instead of pulling rank, he went and tested across browsers on BrowserStack, then publicly reversed himself: "I take it back, I wasn't testing the right thing." Disagreement settled by evidence, not authority, with light banter ("Dark magic." / "Dark magic intensifies.") keeping it human. Elsewhere a reviewer declined without ego, "I'm not familiar with this code, feel free to reassign", showing that admitting the limits of your expertise is normalized, not a loss of face.
Reflection
Rust's lesson is that a great review culture automates everything that isn't judgment. Merge-queue (bors), perf benchmarks (rust-timer), style (tidy), and assignment/state (rustbot) are all mechanical, so the humans spend their attention only on design and correctness, and disagreements get resolved by data rather than seniority. It's the same principle as the rest of this week, scaled up: block on real harm, measure instead of arguing, and keep the person separate from the code.
Discipline: Code review is a series of small criticisms. As the reviewer, lead with the code not the person, tag severity, offer real praise, ask before stating. As the author, default to charity, distinguish "I disagree" from "I don't want the work", say "you're right, fixed" out loud. The deeper principle: separate self from work, your code is not you.
Reflection
The principle that landed hardest was separating self from work, and I felt it most on my own code. Self-reviewing my 4,400-line "Dev" PR meant reading my past self as a hostile stranger and naming a payment bug I'd shipped, then walking back my own overstated first claim ("users never get Pro") once I traced the code. The scarier finding isn't the more honest one.
As a reviewer: lead with the code, not the person, and tag severity so a preference doesn't read as a blocker (a nit and a blocking are different promises). And ask before stating: where I wasn't sure I asked "is there a reason X?" instead of asserting, which mattered, because verifying first killed three confident-but-wrong claims before they reached an author. A wrong "blocking" costs a maintainer time and trust.
As an author: low-ego is mostly respecting the reviewer's attention. On palmier-pro I opened an issue first, kept the PR tiny and test-backed, and said "happy to close if it's not worth it." Receiving feedback is the same muscle pointed inward, which Rust modeled when a reviewer who doubted the author tested on BrowserStack and reversed himself in public ("I take it back"). My code is not me, and the fastest way to look good is to be the first to say "you're right".