From f58f5bb3d8a92f30112d8914ed7006b72e28df05 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Wed, 12 Feb 2025 14:25:46 +0800
Subject: [PATCH] Avoid duplicate SetContextValue call (#33564)

And fix FIXME and TODO
---
 routers/install/install.go            | 1 -
 routers/private/internal.go           | 2 +-
 services/context/context.go           | 1 -
 services/context/package.go           | 4 ++--
 services/contexttest/context_tests.go | 1 -
 templates/org/home.tmpl               | 1 -
 6 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/routers/install/install.go b/routers/install/install.go
index 2cede3685d..8544717f65 100644
--- a/routers/install/install.go
+++ b/routers/install/install.go
@@ -64,7 +64,6 @@ func Contexter() func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
 			base := context.NewBaseContext(resp, req)
 			ctx := context.NewWebContext(base, rnd, session.GetSession(req))
-			ctx.SetContextValue(context.WebContextKey, ctx) // FIXME: this should be removed because NewWebContext should already set it
 			ctx.Data.MergeFrom(middleware.CommonTemplateContextData())
 			ctx.Data.MergeFrom(reqctx.ContextData{
 				"Title":          ctx.Locale.Tr("install.install"),
diff --git a/routers/private/internal.go b/routers/private/internal.go
index 2232c1b78c..55a11aa3dd 100644
--- a/routers/private/internal.go
+++ b/routers/private/internal.go
@@ -87,7 +87,7 @@ func Routes() *web.Router {
 		// FIXME: it is not right to use context.Contexter here because all routes here should use PrivateContext
 		// Fortunately, the LFS handlers are able to handle requests without a complete web context
 		common.AddOwnerRepoGitLFSRoutes(r, func(ctx *context.PrivateContext) {
-			webContext := &context.Context{Base: ctx.Base}
+			webContext := &context.Context{Base: ctx.Base}         // see above, it shouldn't manually construct the web context
 			ctx.SetContextValue(context.WebContextKey, webContext) // FIXME: this is not ideal but no other way at the moment
 		})
 	})
diff --git a/services/context/context.go b/services/context/context.go
index ffce1d617a..5e08fba442 100644
--- a/services/context/context.go
+++ b/services/context/context.go
@@ -166,7 +166,6 @@ func Contexter() func(next http.Handler) http.Handler {
 			ctx.PageData = map[string]any{}
 			ctx.Data["PageData"] = ctx.PageData
 
-			ctx.Base.SetContextValue(WebContextKey, ctx) // FIXME: this should be removed because NewWebContext should already set it
 			ctx.Csrf = NewCSRFProtector(csrfOpts)
 
 			// get the last flash message from cookie
diff --git a/services/context/package.go b/services/context/package.go
index e566b7e532..e32ba3b481 100644
--- a/services/context/package.go
+++ b/services/context/package.go
@@ -154,9 +154,9 @@ func PackageContexter() func(next http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
 			base := NewBaseContext(resp, req)
-			// it is still needed when rendering 500 page in a package handler
+			// FIXME: web Context is still needed when rendering 500 page in a package handler
+			// It should be refactored to use new error handling mechanisms
 			ctx := NewWebContext(base, renderer, nil)
-			ctx.SetContextValue(WebContextKey, ctx) // FIXME: this should be removed because NewWebContext should already set it
 			next.ServeHTTP(ctx.Resp, ctx.Req)
 		})
 	}
diff --git a/services/contexttest/context_tests.go b/services/contexttest/context_tests.go
index 4615c8404b..98b8bdd63e 100644
--- a/services/contexttest/context_tests.go
+++ b/services/contexttest/context_tests.go
@@ -67,7 +67,6 @@ func MockContext(t *testing.T, reqPath string, opts ...MockContextOption) (*cont
 
 	chiCtx := chi.NewRouteContext()
 	ctx := context.NewWebContext(base, opt.Render, nil)
-	ctx.SetContextValue(context.WebContextKey, ctx) // FIXME: this should be removed because NewWebContext should already set it
 	ctx.SetContextValue(chi.RouteCtxKey, chiCtx)
 	if opt.SessionStore != nil {
 		ctx.SetContextValue(session.MockStoreContextKey, opt.SessionStore)
diff --git a/templates/org/home.tmpl b/templates/org/home.tmpl
index db750692bf..bae5db00be 100644
--- a/templates/org/home.tmpl
+++ b/templates/org/home.tmpl
@@ -32,7 +32,6 @@
 						<span class="text">{{svg "octicon-eye"}} {{ctx.Locale.Tr "org.view_as_role" $viewAsRole}}</span>
 						{{svg "octicon-triangle-down" 14 "dropdown icon"}}
 						<div class="menu">
-							{{/* TODO: does it really need to use CurrentURL with query parameters? Why not construct a new link with clear parameters */}}
 							<a href="?view_as=public" class="item {{if not .IsViewingOrgAsMember}}selected{{end}}">
 								{{svg "octicon-check" 14 (Iif (not .IsViewingOrgAsMember) "" "tw-invisible")}} {{ctx.Locale.Tr "settings.visibility.public"}}
 							</a>