Skip to content

Nudge incomplete profiles, gate upload on city and country - #231

Merged
neuromechanist merged 4 commits into
stagingfrom
feat/issue-226-profile-completeness
Aug 3, 2026
Merged

Nudge incomplete profiles, gate upload on city and country#231
neuromechanist merged 4 commits into
stagingfrom
feat/issue-226-profile-completeness

Conversation

@neuromechanist

Copy link
Copy Markdown
Contributor

Closes #226.

Implements the decisions recorded on the issue: a soft nudge everywhere, one
hard gate at upload, and nothing at all in front of browsing or downloading.

Change

src/lib/profile.ts — one definition of "complete", shared by all three
surfaces so they cannot disagree about what they are asking for. Two tiers,
deliberately different in strength:

ProfileNudge.astro on the dashboard — dismissible, never blocking. The
dismissal key is derived from the missing set, so filling in one field
re-surfaces the banner for whatever is still outstanding instead of staying
dismissed. Dismissal is client-side on purpose: it is a UI preference, not
account state, and storing it server-side would need a write endpoint and a
migration for something the user undoes by filling in the form. Storage
failures (private mode) fail open and show the banner.

/upload — server-side gate. The form is withheld, not hidden: a
client-side check would ship the dropzone and then conceal it. Not a redirect
either, because landing on /settings with no explanation reads as a bug. This
is a courtesy check that saves a wasted trip; the backend enforces its own
gate regardless (nemar-cli#1013 Phase 1).

auth-dev.ts — new @nemar.blank persona, matching the existing
@nemar.admin convention. The dev mock previously always issued a complete
profile, so neither of these states was reachable locally.

Verification

Against a dev server with both personas:

dashboard nudge /upload
tester@nemar.blank shown, "missing your city, country, and GitHub handle" gate shown, data-upload-form absent from the HTML
full@example.com absent form present, gate absent

23 new unit tests (10 in profile.test.ts, plus dev-persona coverage). Full
suite 1027 passing, typecheck and lint clean.

Not in scope

No gate at sign-in — the legacy onboarding gate (#129) stays its own track.
I commented on #129 proposing it narrow to the legacy email-PIN first-login
flow, since the decisions here supersede two of its answers.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 3, 2026

Copy link
Copy Markdown

Deploying nemar-website with  Cloudflare Pages  Cloudflare Pages

Latest commit: cadaa79
Status: ✅  Deploy successful!
Preview URL: https://c9728979.nemar-website.pages.dev
Branch Preview URL: https://feat-issue-226-profile-compl.nemar-website.pages.dev

View logs

@neuromechanist

Copy link
Copy Markdown
Contributor Author

Review (two code-reviewer subagents, sonnet)

Both reviews confirmed the gate itself is correct: {uploadBlocked && ...} / {!uploadBlocked && (<form>)} are mutually exclusive, isBlank trims, and there is no other path that renders the form. One reviewer also found a second layer I had not relied on — parseAuthMeResponse in src/middleware.ts already trims and drops blank strings to undefined before AuthUser is built, so whitespace-only values never reach the page in the first place. Both verified every var(--...) in the new rules resolves in tokens.css.

Findings addressed (all of them), in cadaa79

1. canUpload() was dead code, and the gate re-derived the same decision inline. upload.astro computed missingForUpload.length > 0 itself while canUpload was exported and called only by its own test. That is a second definition free to drift, which defeats the stated purpose of the module. The gate now reads const uploadBlocked = !canUpload(session.user), and missingForUpload is used only to name the fields in the copy.

2. The nudge dismissal was not scoped per user. The key was derived from the missing-field set alone, so on a shared browser one account's dismissal silently suppressed the banner for the next account with the same fields missing — and nothing clears localStorage on logout. This is concretely reachable on test.nemar.org, where QA switches between test@nemar.org and the @nemar.test fixtures in one browser. The key now includes the session user id. Verified on a dev server: two blank-profile users get nemar:profile-nudge:dev-tester_nemar_blank:... and nemar:profile-nudge:dev-other_nemar_blank:....

Note this is a different concern from the already-settled "dismissal is client-side, not account state" decision — that was about mechanism, this was about the key.

3. Test gap: whitespace never threaded through canUpload directly. It was only exercised via missingProfileFields. Now that the page derives its gate from canUpload, that path needed its own coverage; added.

4. aria-label="Dismiss" vs "Dismiss this notice" in SiteNotices.astro. Matched the existing wording.

Noted, deliberately not changed

ProfileNudge's focus fallback uses document.querySelector("h1") rather than a prop. Safe today — the component renders only on dashboard.astro, which has exactly one h1, and the :focus{outline:none} rule targets that heading by class. Adding a focusTarget prop for a second caller that does not exist would be speculative; the right time is when it is reused.

Page-level conditional rendering has no vitest coverage, per this repo's convention that page rendering is verified against a live server. Flagging so it is not assumed to be regression-guarded.

On the earlier Cloudflare Pages failure

One reviewer independently reproduced a local bun run build ENOENT during "Rearranging server assets" (a dist/_worker.js/.prerender/chunks rmdir race) on this branch, which succeeded on a clean retry. That is a better explanation than the one I gave: a filesystem race in the build, not anything this PR's code does. The check is green now after merging staging in.

Findings addressed: 4 of 4. Nothing skipped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant