Four more blocking findings
Finishing the five-layer PR review with four more blocking comments, a severity summary, and a request-changes verdict.
Comment 1 set the review’s cadence; now you finish it.
Write the four remaining blocking: comments into reviews/chapter 104.md, then close the file with a ## Summary that totals the severities and a Verdict: request changes line.
The finished file holds five comments in the four-part template, a summary, and a verdict, a review another engineer could act on without a follow-up question.
You are done when that tail sits under the five comments.
## Summary
5 blocking, 0 suggestion, 0 question, 0 nit, 0 praise.
The change is under the ~400-LOC threshold from lesson 1 of chapter 103, so nostructural "split this PR" comment is warranted — it is a single, reviewable surface.
Verdict: request changes — five blocking issues, see comments 1–5.Your mission
Section titled “Your mission”You surface the four findings yourself, file by file in review-stack order. Two of them a junior reviewer reads straight past, so each rewards a reflex worth naming.
You catch the side-effect import by reading the imports, not the function body.
A bare import '@/lib/analytics/page-view-tracker' runs that module’s top-level code; here a void track() fires a fetch the moment the module loads, invisible at the call site.
The missing audit-log write hides for the opposite reason: nothing breaks. The mutation compiles, runs, and rewrites the label, leaving only silence where a compliance row should be. So you read every security-relevant mutation against the canonical event catalog and ask whether it writes an audit-log entry.
The other two carry sharper rules.
User-visible time math always crosses the Temporal seam, because epoch-millisecond arithmetic assumes a 24-hour day and breaks at a DST boundary.
And a value derivable from other state is never useState plus a syncing useEffect: the state itself is the bug, so the fix deletes it rather than memoizing it.
All five findings are blocking: by design, each breaking an established rule with a security, correctness, or contract consequence; that uniformity teaches the blocking-versus-suggesting cut by example.
The pull request also offers a vague handler name, a missing TSDoc, and a nicely co-located module.
Logging all three as blockers to look thorough buries the signal, so they stay at their honest severity, a suggestion:, a nit:, and a praise:, as extra credit.
Keep every comment in the address-the-code-not-the-author voice, and propose each fix in the comment body rather than editing the source.
Since this chapter ships no automated checker, every item below reads untested: confirm each against the four-part comment anatomy and the reference, not a test run.
blocking: comment on the bare side-effect import in src/app/(app)/plan/page.tsx, citing Principle #6 and proposing an explicit named call site or removal if analytics auto-capture already covers the page view.blocking: comment on the Date-arithmetic countdown in src/lib/plan/renewal-countdown.ts, citing SaaS pattern #13 and proposing the Temporal-seam switch to calendar-day math, not millisecond division.blocking: comment on the derived-state effect in src/app/(app)/plan/seat-usage.tsx, citing Principle #7 and the derive-don’t-sync rule, and proposing deletion of the state, the effect, and the resync handler in favor of an inline computation.blocking: comment on the missing audit-log write in src/app/(app)/plan/actions.ts, citing the canonical audit-log event catalog and proposing the logAudit call alongside the write with the organization.plan-label-changed action.## Summary records the severity totals (5 blocking, 0 suggestion, 0 question, 0 nit, 0 praise), the scope note (the surface is small, well under the 400-LOC threshold, so no structural split is warranted), and a one-line pass-order recap.Verdict: request changes line naming the five blocking issues.Coding time
Section titled “Coding time”Write comments 2 through 5, the summary, and the verdict into reviews/chapter 104.md, using reviews/template.md for the four-part shape with the principle-and-pattern cheatsheet open.
Attempt all four before you read on; the reflex sticks only when you make the finding yourself.
Three of these four findings sit on a line you can point at, so practice the pin first: open the slices below and comment on each offending line, and the grader scores you against the one defect a reviewer would flag. Finding 5, the missing audit-log write, is an absence with no line to pin, so it lives only in the prose and the reference.
Click any line to leave a review comment, then press Submit review.
import '@/lib/analytics/page-view-tracker';
import { SeatUsage } from '@/app/(app)/plan/seat-usage';import { getPlanEntitlement } from '@/lib/plan/get-plan-entitlement';import { renewalCountdownDays } from '@/lib/plan/renewal-countdown';import { getSession } from '@/server/session';export const renewalCountdownDays = (renewsAt: string): number => { const millisUntilRenewal = new Date(renewsAt).getTime() - Date.now(); const millisPerDay = 1000 * 60 * 60 * 24; return Math.ceil(millisUntilRenewal / millisPerDay);};export const SeatUsage = ({ seatsAllocated, seatsUsed }: SeatUsageProps) => { const [seatsRemaining, setSeatsRemaining] = useState( seatsAllocated - seatsUsed, );
useEffect(() => { setSeatsRemaining(seatsAllocated - seatsUsed); }, [seatsAllocated, seatsUsed]);
const handlePlanThing = () => { setSeatsRemaining(seatsAllocated - seatsUsed); };The import binds nothing, so it exists only for its side effect: the tracker’s top-level void track() fires a network call the moment the module loads, invisible at the call site.
1000 * 60 * 60 * 24 hard-codes a 24-hour day, but the day crossing a DST transition is 23 or 25 hours, so the count is off by one around the boundary.
Calendar-day math via Temporal makes no such assumption.
seatsRemaining is fully a function of the two props, so holding it in state and resyncing through an effect lets the rendered value lag the props for a frame.
Compute it inline during render and delete the state, the effect, and the resync handler.
Two of these are quiet defects: the bare import and the derived-state effect read as ordinary code until you ask what they do at runtime. The senior reflex is to read imports for smuggled side effects and to treat any state that mirrors props as a bug, not a pattern.
Reference solution and walkthrough
Here are the four comment blocks as they land in reviews/chapter 104.md, under comment 1 from the previous lesson, each in the four-part shape from reviews/template.md.
Finding 2, the side-effect import. You found it by reading line 1, not the page body.
**blocking:** `src/app/(app)/plan/page.tsx` L1 — the bare `import '@/lib/analytics/page-view-tracker'` runs that module's top-level `void track()`, firing a `fetch` at import time; the side effect is invisible at the call site.Principle/pattern: Principle #6 explicit-over-magic (chapter 029 / chapter 042).Action: drop the bare import and make the page view an explicit `trackPlanPageView()` call at a real boundary, or remove it if analytics auto-capture already covers the page view.Finding 3, the Date arithmetic. The fix names the Temporal seam, not a vague warning.
**blocking:** `src/lib/plan/renewal-countdown.ts` L8-11 — `new Date(renewsAt).getTime() - Date.now()` divided by `1000*60*60*24` assumes a fixed 24-hour day, so it returns the wrong day count across a DST boundary, and it reads the machine clock.Principle/pattern: SaaS pattern #13 time/dates/timezones (Chapter 083).Action: use the Temporal seam (`src/lib/temporal.ts`): `plainDateFromString(renewsAt).until(today, { largestUnit: 'days' }).days`, working in calendar days rather than millisecond division.Finding 4, the derived-state effect. The action deletes; it does not memoize.
**blocking:** `src/app/(app)/plan/seat-usage.tsx` L15-25 — `seatsRemaining` is held in `useState` and resynced from the `seatsAllocated`/`seatsUsed` props via `useEffect` (plus a `handlePlanThing` handler that resyncs on click); the rendered value can lag the props for a frame.Principle/pattern: Principle #7 impossible-states-unrepresentable / derive-don't-sync (Chapter 025).Action: delete the state, the effect, and the resync handler; render `seatsAllocated - seatsUsed` inline.Finding 5, the missing audit-log write. The absence is the finding, and the write rides the same mutation.
**blocking:** `src/app/(app)/plan/actions.ts` L33 — the `planLabel` write records nothing to the audit log; the compliance trail is silent on a security-relevant mutation.Principle/pattern: canonical audit-log event catalog (lesson 5 of chapter 057 / lesson 3 of chapter 081).Action: add `logAudit({ orgId: ctx.orgId, actorUserId: ctx.userId }, { action: 'organization.plan-label-changed', subjectType: 'organization', subjectId: ctx.orgId, payload: { previousLabel, nextLabel } })` alongside the write (this falls out naturally once the action is wrapped in `authedAction` per finding 1).The bonus block is extra credit, the senior reach rather than a requirement, with every item at its honest severity.
**suggestion:** `src/lib/plan/get-plan-entitlement.ts` L8 — the exported entitlement read has no TSDoc; it's a cross-module read surface other features will call.Principle/pattern: cross-module documentation (lesson 1 of chapter 102).Action: add a one-paragraph TSDoc with summary, `@param orgId`, `@returns`, and a note that callers must `updateTag(orgPlanEntitlementTag(orgId))` after mutating plan state.
**nit:** `src/app/(app)/plan/seat-usage.tsx` L23 — `handlePlanThing` doesn't name its intent.Principle/pattern: Principle #4 name-for-intent.Action: rename or, better, delete it with the derived-state fix above.
**praise:** `src/lib/plan/` + `src/app/(app)/plan/` — schema, action, read function, and component are co-located by feature.Principle/pattern: Principle #1 co-locate-by-feature.Action: none — naming the choice so the author knows the pattern landed.The summary then totals the five severities, notes that the 400-LOC threshold did not fire, recaps the stack order in which the findings surfaced, and ends on a one-line verdict.
## Summary
5 blocking, 0 suggestion, 0 question, 0 nit, 0 praise.
The change is under the ~400-LOC threshold from lesson 1 of chapter 103, so nostructural "split this PR" comment is warranted — it is a single, reviewable surface.
Pass-order recap: the five blockers surfaced top-down on the review stack —correctness/security first (the auth bypass, the silent audit trail), thenprinciples (the magic side-effect import, the derived-state effect), thenpatterns (the `Date` time math); tests/contracts and style found nothing blocking.
Verdict: request changes — five blocking issues, see comments 1–5.Finding 4’s action deletes the state; it does not reach for useMemo.
The state itself is the bug: seatsRemaining is a pure function of two props, so storing it at all makes the impossible state representable, a remaining count that disagrees with allocated-minus-used.
Memoizing would keep the redundant copy and merely hide the lag; instead, compute the value inline during render and delete the useState, the useEffect, and the resync handler in one move.
This is the derive-don’t-sync rule from the “you probably don’t need an effect” lesson: cite it, don’t re-teach it.
Finding 5’s logAudit is written alongside the same mutation.
In the database framing that means the same transaction, so a rollback unwinds the audit row together with the write and the two can never disagree; logging after the redirect would orphan the event if the write later fails, the failure mode the audit-log policy lesson and the append-only audit log exist to prevent.
It also interlocks with finding 1: once updatePlanLabel is wrapped in authedAction per comment 1, ctx.orgId and ctx.userId are already in scope, so the logAudit call falls out without its own hand-rolled session read.
The bonus findings stay suggestion:, nit:, and praise:.
Those are the right shapes for a subjective choice, a name you would prefer, and a structure worth acknowledging; promoting any to blocking: would blunt the very cut the reference scores.
The chapter teaches restraint by example: five sharp blockers another engineer can act on beat twelve comments that bury them, the labels and the cut both from the comment-that-lands lesson.
Across all five, the comment names the rule and links the owning lesson where the team agreed on it; it does not paste the lesson back in.
The canonical source behind finding 4's derive-don't-sync rule — compute during render, don't store and resync.
The PlainDate.until calendar-day arithmetic your finding 3 action proposes in place of millisecond division.
Moment of truth
Section titled “Moment of truth”A real PR review has no answer key, and this chapter ships no checker, so you verify by hand against the comment anatomy, the way you would read a teammate’s.
Open reviews/chapter 104.md and confirm the shape: five comment blocks, comment 1 from the previous lesson plus the four you wrote here, each pinned to a file and line and carrying all four parts, then a ## Summary with severity totals and a scope note and a closing Verdict: line.
Now hand-check the parts that take judgment.
blocking:, not suggestion: — any mis-label loses the severity-credit half on that finding even when the defect is correctly located.logAudit call names the organization.plan-label-changed action with its payload shape and sits alongside the write, not after a redirect.Don’t open solution/reviews/chapter 104.md yet: the side-by-side grade against the reference comes next lesson, and reviewing under no-peeking is the reflex this project trains.
For now, confirm your file stands on its own.