charles/claude-hooks
1
0
Fork 0
Code Issues 10 Pull requests Projects Releases Packages 1 Wiki Activity Actions

fix(login): post-#538 UX bonuses — Forgejo scope, login-loop, native logout #541

Merged
charles merged 2 commits from fix/login-bonus-fixes into main 2026-04-28 20:40:26 +00:00
Conversation 2 Commits 2 Files changed 7 +42 -34
claude-desktop commented 2026-04-28 19:20:36 +00:00
Collaborator
Copy link

Three small fixes the operator hit during sign-in immediately after #538 merged. Each is independent; all live tested against the production deployment today.

1. forgejo-oauth.ts — add read:user scope

FORGEJO_SCOPES = "read:repository write:repository write:hook" — fine for repo + webhook operations but the callback's GET /api/v1/user profile fetch requires read:user. Without it the operator's first sign-in attempt logged [forgejo-oauth] profile fetch failed: HTTP 403 three times and bounced them to /login?error=profile_failed. Adds read:user to the scope list.

Note for operators: revoke + re-grant the existing Forgejo OAuth app authorisation once after merging — Forgejo caches the prior narrower scope set on the grant.

2. login.tsx — break the login-page-stuck loop

buildInitHref captured window.location.pathname as the OAuth return param. When the operator was on /app/login (the natural entry point) that captured path WAS /app/login, so the callback redirected back to the login page after a successful sign-in and the SPA stayed put. Falls back to / when the current path is the login surface; deep-link semantics preserved everywhere else. Drops the typeof window === "undefined" SSR-defence cruft — this is a pure-client SPA, no SSR.

3. app-shell.tsx + useFlowAuth.ts — native logout button

The header logout link relied on cfg.auth.authelia_logout_url exposed via /whoami. With the Authelia layer ripped on the proxy side earlier today (proxmox-iac/services/caddy/.../Caddyfile.j2), that field is dead — clicking Logout did nothing. Replaces the anchor with <form method="POST" action="/logout"> + a styled submit button — hits the existing session-clear endpoint that already 302s back to /login. Drops logout_url from the WhoamiResponse shape in both AppShell and useFlowAuth.

4. __root.tsx — bidirectional /whoami redirect

Extends the mount-time probe added in #538 so it also redirects "authed + on the login page → /". Mirrors fix #2 on the SPA side: even if the operator lands on /app/login while already authed (back button, bookmark, stale tab), they get bounced to the dashboard instead of staring at the login form. Uses TanStack's useNavigate to keep router state.

Verification

  • bun run typecheck clean
  • Operator went through full Forgejo OAuth flow on production deployment, lands on /app/monitor after sign-in
  • Logout button clears the session row + cookie, 302s to /login
  • Hitting /app/login while authed redirects to /

Test plan

  • Reviewer: revoke their own grant on forge.jacquin.app/user/settings/applications, re-grant via Sign in with Forgejo button, confirm dashboard renders
  • Reviewer: click Logout from header, confirm session row in operator_sessions is gone and cookie is cleared

Out of scope

  • /api/* boundary refactor (closed PR #539, will reopen as a separate issue)
  • auth.authelia_logout_url schema removal (#540 covers the bigger config split)
  • Removing the legacy auth.operator_user / auth.trust_proxy post-Authelia (separate audit)
Three small fixes the operator hit during sign-in immediately after #538 merged. Each is independent; all live tested against the production deployment today. ## 1. `forgejo-oauth.ts` — add `read:user` scope `FORGEJO_SCOPES = "read:repository write:repository write:hook"` — fine for repo + webhook operations but the callback's `GET /api/v1/user` profile fetch requires `read:user`. Without it the operator's first sign-in attempt logged `[forgejo-oauth] profile fetch failed: HTTP 403` three times and bounced them to `/login?error=profile_failed`. Adds `read:user` to the scope list. **Note for operators:** revoke + re-grant the existing Forgejo OAuth app authorisation once after merging — Forgejo caches the prior narrower scope set on the grant. ## 2. `login.tsx` — break the login-page-stuck loop `buildInitHref` captured `window.location.pathname` as the OAuth `return` param. When the operator was on `/app/login` (the natural entry point) that captured path WAS `/app/login`, so the callback redirected back to the login page after a successful sign-in and the SPA stayed put. Falls back to `/` when the current path is the login surface; deep-link semantics preserved everywhere else. Drops the `typeof window === "undefined"` SSR-defence cruft — this is a pure-client SPA, no SSR. ## 3. `app-shell.tsx` + `useFlowAuth.ts` — native logout button The header logout link relied on `cfg.auth.authelia_logout_url` exposed via `/whoami`. With the Authelia layer ripped on the proxy side earlier today (`proxmox-iac/services/caddy/.../Caddyfile.j2`), that field is dead — clicking Logout did nothing. Replaces the anchor with `<form method="POST" action="/logout">` + a styled submit button — hits the existing session-clear endpoint that already 302s back to `/login`. Drops `logout_url` from the `WhoamiResponse` shape in both `AppShell` and `useFlowAuth`. ## 4. `__root.tsx` — bidirectional `/whoami` redirect Extends the mount-time probe added in #538 so it also redirects "authed + on the login page → `/`". Mirrors fix #2 on the SPA side: even if the operator lands on `/app/login` while already authed (back button, bookmark, stale tab), they get bounced to the dashboard instead of staring at the login form. Uses TanStack's `useNavigate` to keep router state. ## Verification - [x] `bun run typecheck` clean - [x] Operator went through full Forgejo OAuth flow on production deployment, lands on `/app/monitor` after sign-in - [x] Logout button clears the session row + cookie, 302s to `/login` - [x] Hitting `/app/login` while authed redirects to `/` ## Test plan - [ ] Reviewer: revoke their own grant on `forge.jacquin.app/user/settings/applications`, re-grant via `Sign in with Forgejo` button, confirm dashboard renders - [ ] Reviewer: click Logout from header, confirm session row in `operator_sessions` is gone and cookie is cleared ## Out of scope - `/api/*` boundary refactor (closed PR #539, will reopen as a separate issue) - `auth.authelia_logout_url` schema removal (#540 covers the bigger config split) - Removing the legacy `auth.operator_user` / `auth.trust_proxy` post-Authelia (separate audit)
fix(login): post-merge UX bonuses operator hit during sign-in
Some checks failed
qa / qa (pull_request) Failing after 12m14s
Details
qa / dockerfile (pull_request) Successful in 13s
Details
12a499cce0 
Three small fixes that surfaced as the operator went through the OAuth
flow on a fresh deployment after #538 landed.

## forgejo-oauth — read:user scope

`FORGEJO_SCOPES` was `read:repository write:repository write:hook` —
fine for repo + webhook ops but `GET /api/v1/user` (the profile fetch
the callback runs to bind the session to an account login) needs
`read:user`. Without it the callback exited 302 to `/login?error=
profile_failed` and the operator stayed stuck on the login page.
Adds `read:user` to the front of the list.

After this lands, operators MUST revoke their existing OAuth grant
on the forge once (Settings → Applications → Authorised → Revoke)
and re-grant — Forgejo cached the old narrower scope set.

## login.tsx — break the login-page-stuck loop

`buildInitHref` captured `window.location.pathname` as the OAuth
`return` param. When the operator was already on `/app/login` (the
expected entry point), that captured path WAS `/app/login`, so the
callback redirected back to the login page after a successful sign-in
and the SPA stayed put. Falls back to `/` when the current path is
the login surface itself; otherwise keeps the deep-link semantics.
Drops the `typeof window === "undefined"` SSR-defence cruft — this is
a pure-client SPA, no SSR.

## app-shell + useFlowAuth — native logout button

The header logout link relied on `cfg.auth.authelia_logout_url` from
`/whoami`. With the Authelia layer ripped on the proxy side today,
that field is dead. Replaces the anchor with `<form method="POST"
action="/logout">` + a submit button — hits the existing
session-clear endpoint that already 302s back to `/login`.
Drops `logout_url` from the `WhoamiResponse` shape in both
`AppShell` and `useFlowAuth`.

## __root.tsx — bidirectional /whoami redirect

Extends the post-#538 mount-time probe so it also redirects
authed-on-login-page → `/`. Mirrors the login-loop fix above on
the SPA side: even if a user lands on `/app/login` while already
authed (back button, bookmark), they get bounced to the dashboard
instead of staring at the login form.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
test(oauth): update scope assertion after read:user addition
All checks were successful
qa / qa (pull_request) Successful in 13m35s
Details
qa / dockerfile (pull_request) Successful in 14s
Details
f7b4308687 
Two assertions still expected the old `read:repository write:repository
write:hook` scope set; widen them to match the new
`read:user read:repository write:repository write:hook`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
claude-desktop commented 2026-04-28 20:33:37 +00:00
Author
Collaborator
Copy link

Reviewer concern verified — no regression

Reviewer flagged the unconditional __root.tsx /whoami probe as a potential test-breaker. Ran the full suite with the test bypass set:

CLAUDE_HOOKS_DISABLE_AUTH=1 bun test apps/server
 2509 pass
 4 fail

The 4 fails are pre-existing on main, all unrelated to the PR:

  • 3× session JSONL pruning > … (FS / time-based)
  • 1× foreman session CRUD > listSessions returns newest-first … (timing)

No __root.tsx / /whoami test broke. Probe runs at component mount; tests that don't render the SPA root never fire it.

Web suite (bun x vitest run in apps/web) has the same theme-toggle.test.tsx + theme.test.ts localStorage.clear is not a function flake we've been seeing on main since this morning — also pre-existing.

Verdict: ready to merge once a human gives the LGTM. Reviewer comment thread is resolved by verification rather than a code change.

### Reviewer concern verified — no regression Reviewer flagged the unconditional `__root.tsx` `/whoami` probe as a potential test-breaker. Ran the full suite with the test bypass set: ``` CLAUDE_HOOKS_DISABLE_AUTH=1 bun test apps/server 2509 pass 4 fail ``` The 4 fails are pre-existing on `main`, all unrelated to the PR: - 3× `session JSONL pruning > …` (FS / time-based) - 1× `foreman session CRUD > listSessions returns newest-first …` (timing) No `__root.tsx` / `/whoami` test broke. Probe runs at component mount; tests that don't render the SPA root never fire it. Web suite (`bun x vitest run` in `apps/web`) has the same `theme-toggle.test.tsx` + `theme.test.ts` `localStorage.clear is not a function` flake we've been seeing on `main` since this morning — also pre-existing. Verdict: ready to merge once a human gives the LGTM. Reviewer comment thread is **resolved by verification** rather than a code change.
charles deleted branch fix/login-bonus-fixes 2026-04-28 20:40:27 +00:00
reviewer approved these changes 2026-04-28 21:33:08 +00:00
reviewer left a comment
Copy link

Scope and correctness all check out. CI green.

  • read:user scope addition is the right fix; profile fetch on /api/v1/user always required it.
  • Login-loop fix in login.tsx and mirror redirect in __root.tsx are consistent — same three isLoginPath conditions in both places.
  • Native <form method="POST" action="/logout"> is cleaner than the dead Authelia URL anchor; data-testid preserved.
  • logout_url removal from WhoamiResponse in both app-shell.tsx and useFlowAuth.ts is clean.
  • Tests updated in lockstep with the scope string change.

Nit (non-blocking): routeTree.gen.ts carries an unexplained settings-route rename (SettingsRoute → SettingsIndexRoute) not mentioned in the PR description — looks like a generated-file side-effect of unrelated route restructuring. Harmless.

Scope and correctness all check out. CI green. - `read:user` scope addition is the right fix; profile fetch on `/api/v1/user` always required it. - Login-loop fix in `login.tsx` and mirror redirect in `__root.tsx` are consistent — same three `isLoginPath` conditions in both places. - Native `<form method="POST" action="/logout">` is cleaner than the dead Authelia URL anchor; `data-testid` preserved. - `logout_url` removal from `WhoamiResponse` in both `app-shell.tsx` and `useFlowAuth.ts` is clean. - Tests updated in lockstep with the scope string change. Nit (non-blocking): `routeTree.gen.ts` carries an unexplained settings-route rename (`SettingsRoute` → `SettingsIndexRoute`) not mentioned in the PR description — looks like a generated-file side-effect of unrelated route restructuring. Harmless.
Sign in to join this conversation.
Reviewers
No reviewers
reviewer
Labels
Clear labels   
area:agents
Agent types, pool scheduling, per-instance config

  
area:dashboard
Dashboard UI and observability surfaces

  
area:database
DB layer — schema, migrations, ORM, raw SQL

  
area:design
UI/UX mockup work — routes to designer agent

  
area:design-review
Design review dispatch — routes to design-reviewer agent

  
area:flows
Flow runner — YAML loader, executor, op registry, expression eval

  
area:infra
Deployment, isolation, containers, systemd units

  
area:meta
Tracking, scaffolding, project setup

  
area:security
Security — routes to reviewer-security (opus)

  
area:sessions
Session-id store, Claude SDK resume logic

  
area:webhook
Forgejo webhook routing and handlers

  
area:workdir
Clone cache, worktrees, git identity

  
security
Security-sensitive issue

  
type:bug
Bug

  
type:chore
Chore

  
type:meta
Tracking or decisions, not implementation work

  
type:user-story
User story

No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/claude-hooks!541
No description provided.