Skip to content

chore(web): correct stale and redundant comments in auth and signup modules - #9253

Merged
iscekic merged 2 commits into
mainfrom
drain/auth-signup-comment-cleanup
Oct 30, 2026
Merged

iscekic merged 2 commits into
mainfrom
drain/auth-signup-comment-cleanup

Conversation

@iscekic

@iscekic iscekic commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Evidence

Evidence: demonstrated — the stale claims were checked against the source on main and corrected in place, and the change is proven comment-only: for each of the five touched files, the TypeScript printer output (comments removed) is byte-identical before and after, so no code, type or behavior changes. The affected suites pass unchanged: apps/web Jest device-sessions, native-admission, email-signin-eligibility plus packages/web-shared blacklist-domains-config and user/server (5 suites, 205 tests), and scripts/typecheck-all.sh --changes-only reports the affected packages clean. Both touched areas are comment-only library code with no runtime surface of its own, so there is no separate UI or API path to drive; the suites exercise the functions the comments describe.

Each item and the source that established it:

  1. rotateRefreshToken returned no null: its signature is a discriminated union ending in { ok: false; error: 'INVALID_REFRESH_TOKEN' | 'SESSION_REVOKED' | 'USER_BLOCKED' }, and every failure path returns that arm. The "returns null" sentence is gone.
  2. validateAdmissionPayload rejects an iOS payload without keyId for both attestation and assertion, matching the type's own field doc; the label that named only assertion is removed, so the requirement is stated once (inline at the iOS check and on the field).
  3. Three labels in the same function restated the condition on the next line; removed. The payload label stays because base64 encoding is not visible from a non-empty-string check.
  4. The checkDomainSignInEligibility doc listed sign-in paths but missed the passkey branch of app/api/auth/native/token/route.ts, which also calls it; the doc now states the invariant without an enumeration that goes stale. The same paragraph now points at lib/user/next-auth-options.ts for the mirrored inline checks, which is where the signIn callback lives.
  5. DEFAULT_BLOCKED_SIGNUP_TLDS documents only .shop and .top, which is what the array holds and what the env fallback returns.
  6. A trailing comment restated the session.isNewUser assignment while the sibling assignments carry none; removed, and the neighboring isAdmin comment that documents the || false invariant is kept.

Production prerequisites

None.

Proxy answers

The owner's proxy gave these answers in the owner's place.

  1. Question: keyId comment: apps/web/src/lib/auth/native-admission.ts:175 reads // keyId: optional string (required for iOS assertion) and contradicts the iOS check at 180-182 (keyId required for attestation and assertion) and the type doc at 107. The finding's recommendation is to delete the line; the work-item brief says to correct it to say both iOS kinds, and the inline comment at 181 (// iOS requires keyId for both attestation and assertion) would then say the same thing twice. How should line 175 be handled?
    Answer: Delete line 175
    Confidence: 0.66 · Precedent: Speed and least work: 'do the least amount of work possible, with no drop in quality' (session 7f3470e0ba6f). The brief's goal is to remove stale and restating comments, and the comment at 181 and the type doc at 107 already state the correct requirement.

Comment-only cleanup of five files. The TypeScript printer output of each
file is identical before and after, so no behavior changes.

- device-sessions: drop the stale "returns null with an error code" line;
  rotateRefreshToken returns a discriminated union whose failure arm is
  `{ ok: false, error }`.
- native-admission: remove the keyId label that named only iOS assertion
  while the check requires it for attestation too, and remove three
  labels that restate the conditions below them. The payload label stays.
- email-signin-eligibility: stop enumerating sign-in paths in the
  checkDomainSignInEligibility doc; the native passkey branch also calls
  the shared core, so the invariant is stated without a list.
- blacklist-domains-config: describe only the TLDs
  DEFAULT_BLOCKED_SIGNUP_TLDS holds.
- user/server: drop the comment that restates the isNewUser assignment.
@iscekic iscekic added drain Work item from the findings drain janitor:comment-remover Remove redundant or dead comments labels Oct 30, 2026
@kilo-code-bot

kilo-code-bot Bot commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

The incremental change is comment-only and fixes the lone prior finding: the checkDomainSignInEligibility doc now points at lib/user/next-auth-options.ts, which does contain the signIn callback with the mirrored blacklist, blocked-TLD, and SSO checks.

Assumptions: [INFERENCE] The TypeScript-printer byte-identity claim in the description was not re-run; the incremental diff shows only a comment line changed, so no code path was exercised.

Files Reviewed (1 changed in this pass)
  • apps/web/src/lib/auth/email-signin-eligibility.ts - 0 issues (new cross-reference verified against the signIn callback at lib/user/next-auth-options.ts:968)
Previously reviewed, unchanged
  • apps/web/src/lib/auth/device-sessions.ts - 0 active findings
  • apps/web/src/lib/auth/native-admission.ts - 0 active findings
  • packages/web-shared/src/lib/blacklist-domains-config.ts - 0 active findings
  • packages/web-shared/src/lib/user/server.ts - 0 active findings
Previous Review Summary (commit a18f136)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit a18f136)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1

Executive Summary

The change is comment-only and every correction matches the current source, except one stale cross-reference path that survived the email-signin-eligibility reflow.

Assumptions: [INFERENCE] The TypeScript-printer byte-identity claim in the description was not re-run; the diff itself shows only comment lines changed, so no code path was exercised.

Issue Details (click to expand)

SUGGESTION

File Line Issue
apps/web/src/lib/auth/email-signin-eligibility.ts 118 Doc still points the mirrored signIn checks at lib/user/server.ts, but the callback moved to lib/user/next-auth-options.ts in #6894.
Files Reviewed (5 files)
  • apps/web/src/lib/auth/device-sessions.ts - 0 issues (removed "returns null" line matches the discriminated union failure arm)
  • apps/web/src/lib/auth/email-signin-eligibility.ts - 1 issue (stale path reference)
  • apps/web/src/lib/auth/native-admission.ts - 0 issues (removed labels match the iOS keyId check at 173-176 and the field doc at 107)
  • packages/web-shared/src/lib/blacklist-domains-config.ts - 0 issues (DEFAULT_BLOCKED_SIGNUP_TLDS is ['shop', 'top'] and is the env fallback)
  • packages/web-shared/src/lib/user/server.ts - 0 issues (only the redundant trailing comment was dropped)

Reviewed by deepseek-flash-latest · Input: 24K · Output: 1.5K · Cached: 163.2K

Review guidance: REVIEW.md from base branch main

@iscekic iscekic added merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. labels Oct 30, 2026
The checkDomainSignInEligibility doc named lib/user/server.ts as the file
holding the mirrored inline checks. Those checks live in the signIn
callback of lib/user/next-auth-options.ts, so the reference now names that
file. Comment-only.
@iscekic iscekic added merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. and removed merge:blocked:kilobot-not-ready Review bot has not recommended merge at this head merge:bot Bot-owned review; merge when ready once readiness passes and a review approves. labels Oct 30, 2026
@iscekic
iscekic added this pull request to the merge queue Oct 30, 2026
Merged via the queue into main with commit 7bde61d Oct 30, 2026
58 checks passed
@iscekic
iscekic deleted the drain/auth-signup-comment-cleanup branch October 30, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drain Work item from the findings drain janitor:comment-remover Remove redundant or dead comments merge:bot Bot-owned review; merge when ready once readiness passes and a review approves.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants