Four more blocking findings
Comment 1 set the cadence. The auth bypass announced itself the moment you read updatePlanLabel against the wrapper it skipped — a hand-rolled session check is a shape your eye now catches. The four findings left in the /plan surface are not all that loud, and that is the point of this lesson. Your goal here is to finish the review: 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. By the end the file holds five comments in the four-part template, a summary, and a verdict — a review another engineer could act on without asking you a single follow-up question.
There is no screen to show you, because the deliverable is a text file. What “done” looks like is the tail of that file: the summary block and the verdict, sitting 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”With the cadence set, you surface the four remaining findings yourself, working file-by-file in review-stack order rather than top-down on the file list. Two of them reward a specific reflex worth naming, because each is the kind of defect a junior reviewer reads straight past. The side-effect import is the one you find by reading the imports, not the function body: a bare import '@/lib/analytics/page-view-tracker' at the top of a server component is an instruction to open that module and read its top-level scope, and when there is executable code at module scope that touches the world — here a void track() that fires a fetch — the import is the call. The missing audit-log write hides for the opposite reason: nothing breaks. The mutation compiles, runs, and rewrites the label, and the only thing wrong is the silence where a compliance row should be — so you read every security-relevant mutation against the canonical event catalog and ask, flatly, “does this write an audit-log entry?” The other two carry sharper rules than juniors reach for. User-visible time math crosses Temporal with no exceptions — not “be careful with dates,” but a hard line, 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 violates an established rule with a security, correctness, or contract consequence, and that uniformity is the lesson teaching the blocking-versus-suggesting cut by example. The temptation now is to pad. The surface has a vague handler name, a missing TSDoc, a nicely co-located module — and a junior review reflexively logs all of them to look thorough. Resist it. A review drowning in nits buries the signal; the right severity mix beats the most comments. The bonus findings (a TSDoc suggestion:, a naming nit:, a co-location praise:) are extra credit, never required, and they stay at their honest severity rather than getting inflated to blockers. Keep every comment in the address-the-code-not-the-author voice. Editing the source stays out of scope — the fix you propose lives in the comment body — and the ADR waits for the next lesson.
Because this chapter ships no automated checker, every item below reads untested. You confirm each one against the four-part comment anatomy and the reference, not against 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 against the brief above. Use reviews/template.md for the four-part shape and keep the principle-and-pattern cheatsheet open. Attempt all four before you read on — the reflexes only stick if you do the finding yourself first.
Three of these four findings sit on a line you can point at, so before the reference, practice the pin. Open the slices below and leave a comment on the offending line of each; the grader scores your comment against the one defect a reviewer would flag. (Finding 5 — the missing audit-log write — is an absence, which cannot be pinned to a line, 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 has no binding, so the only reason it exists is 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; the day that crosses a DST transition is 23 or 25 hours, so the count is off by one around the boundary. Calendar-day math via Temporal has 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. The fix is to 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 actually 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 is the four-part shape from reviews/template.md: severity and pinned location with the observation, the principle or pattern with its lesson ID, and the action.
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 after they change.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. It is the senior reach, not a requirement, and every item stays 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.Then the file closes. The summary totals the five severities, the scope note records that the 400-LOC threshold did not fire, the pass-order recap shows the findings surfaced in stack order, and the verdict is one line.
## 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.A few decisions worth their own sentences.
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 is what makes the impossible state — a remaining count that disagrees with allocated-minus-used — representable in the first place. Memoizing the same value would keep the redundant copy and merely hide the lag; the fix is to compute it 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 this means inside the same transaction, so that a rollback unwinds the audit row together with the write — the log and the data can never disagree. Logging after the redirect would orphan the event if the write later fails, which is the failure mode the audit-log policy lesson and the append-only audit log exist to prevent. Note too that this finding interlocks with finding 1: once updatePlanLabel is wrapped in authedAction per comment 1, ctx.orgId and ctx.userId are already in scope, and the logAudit call falls out naturally rather than needing its own hand-rolled session read.
The bonus findings stay suggestion:, nit:, and praise:. They are the right shape for a subjective choice — a name you would prefer, a doc you would add, a structure you want to acknowledge — and promoting any of them to blocking: would blunt the very cut the reference scores. The chapter teaches restraint by example: five sharp blockers that another engineer can act on beat twelve comments that bury them. The severity labels and that cut come from the comment-that-lands lesson.
For Principle #6, the Temporal seam, derive-don’t-sync, the audit-log catalog, and the severity labels, the comments cite the owning lesson by ID and link out. A review comment names the rule and points at 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”There is no automated checker for this chapter — lesson-verification/ ships empty, with only a .gitkeep, so there is no pnpm test:lesson 3 to run. A real PR review has no answer key either, which is exactly why the verification here is a hand-check: you read your own file against the anatomy the way you would read a teammate’s.
Open reviews/chapter 104.md and confirm the shape first. It should hold five comment blocks — comment 1 from the previous lesson plus the four you wrote here — each pinned to a file and a line and each carrying all four parts. Below them, a ## Summary with severity totals and a scope note, and a closing Verdict: line.
Then hand-check the parts that take judgment, ticking each off as you go.
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.The review file is now complete and internally consistent: five blockers, a summary, a verdict. Resist the urge to open solution/reviews/chapter 104.md to check your work — the self-grade against the reference happens in the next lesson, where both deliverables are scored side by side, and running this pass under “no peeking” is the reflex the whole project trains. For now you only confirm your file stands on its own.