The new package internal/providers builds these parts from the config:
the cache
the fetcher
the processor
the image service
the session manager
the encrypted URL generator
pixad's fx graph provides them with fx.Provide(providers.Constructors()...), and handlers.New receives them built. internal/app and the server tests use that one list. Handlers no longer holds the database, the fetcher or the metrics registry, and initImageService is gone.
Each setting's default is declared in internal/config. httpfetcher.DefaultConfig(), the fetcher's copies of setting defaults and imageprocessor.DefaultMaxInputBytes are gone; both constructors take explicit values. internal/providers works out the default cache_max_bytes with config.DefaultCacheMaxBytes before it builds the cache. The cache's MaxBytes is the one value for the disk cache limit, and 0 turns the disk cache off.
Not visible in the diff:
Shutdown order is unchanged: the server stops, then the cache writes its pending access times, then the database closes.
The fetcher's settings are a value of their own in the fx graph. The image proxy test replaces only their dial function, with fx.Decorate.
The user agent and the accepted content types are now fixed in httpfetcher; they were never settings.
Disclosures:
Deviation: database.New opens the database itself, not in its start hook, so the session manager and the default cache_max_bytes can read it while fx builds them. The Go HTTP server conventions show the start hook.
Tests of moved code moved to internal/providers and internal/config; tests of the removed defaults went with them.
Model: opus-5-5
Closes https://git.eeqj.de/sneak/pixa/issues/313.
The new package `internal/providers` builds these parts from the config:
- the cache
- the fetcher
- the processor
- the image service
- the session manager
- the encrypted URL generator
pixad's fx graph provides them with `fx.Provide(providers.Constructors()...)`, and `handlers.New` receives them built. `internal/app` and the server tests use that one list. `Handlers` no longer holds the database, the fetcher or the metrics registry, and `initImageService` is gone.
Each setting's default is declared in `internal/config`. `httpfetcher.DefaultConfig()`, the fetcher's copies of setting defaults and `imageprocessor.DefaultMaxInputBytes` are gone; both constructors take explicit values. `internal/providers` works out the default `cache_max_bytes` with `config.DefaultCacheMaxBytes` before it builds the cache. The cache's `MaxBytes` is the one value for the disk cache limit, and 0 turns the disk cache off.
Not visible in the diff:
- Shutdown order is unchanged: the server stops, then the cache writes its pending access times, then the database closes.
- The fetcher's settings are a value of their own in the fx graph. The image proxy test replaces only their dial function, with `fx.Decorate`.
- The user agent and the accepted content types are now fixed in `httpfetcher`; they were never settings.
Disclosures:
- Deviation: `database.New` opens the database itself, not in its start hook, so the session manager and the default `cache_max_bytes` can read it while fx builds them. The Go HTTP server conventions show the start hook.
- Tests of moved code moved to `internal/providers` and `internal/config`; tests of the removed defaults went with them.
Model: opus-5-5
clawbot
self-assigned this 2026-10-26 11:32:56 +01:00
internal/server/login_rate_limit_internal_test.go, newImagePipeline (used by the image proxy, login and logout tests). It copies the six providers of internal/app/providers.go field by field. That is the same copy #313 removes from initImageService. It also differs from them: the disk cache is off when cache_max_bytes is omitted, and the cache's background goroutines never start. On next these tests ran pixad's own wiring through handlers.New; now they run the copy. So a defect in the providers pixad runs, such as the image service built with AllowHTTP false, passes the whole suite. That internal/server cannot import internal/app is not a reason to copy the code. Acceptable: the providers, including the work-out of the default cache_max_bytes, live in one package that both internal/app and the server tests import, and that package's doc says it builds the image pipeline from the config. The server tests use those providers and replace only the fetcher's dial function. The internal/app test takes the provider list from the same place instead of repeating it.
internal/app/providers.go: newCache takes five positional arguments, newImageService five and newSessionManager two. Every other fx constructor in pixa takes (lc fx.Lifecycle, params Params) with an fx.In struct. The Go HTTP server conventions' factory function pattern and the Go style guide's rule that constructors take a Params struct both require this. Acceptable: each provider with more than one dependency takes a params struct with fx.In.
Judgement call: database.New opening the database in its constructor is accepted as the disclosed deviation, not a finding. Keeping the open in the start hook would mean setting the cache's limit or the session manager's state after construction, which the issue rules out.
Model: opus-5-5
FAIL: needs-rework.
1. `internal/server/login_rate_limit_internal_test.go`, `newImagePipeline` (used by the image proxy, login and logout tests). It copies the six providers of `internal/app/providers.go` field by field. That is the same copy https://git.eeqj.de/sneak/pixa/issues/313 removes from `initImageService`. It also differs from them: the disk cache is off when `cache_max_bytes` is omitted, and the cache's background goroutines never start. On `next` these tests ran pixad's own wiring through `handlers.New`; now they run the copy. So a defect in the providers pixad runs, such as the image service built with `AllowHTTP` false, passes the whole suite. That `internal/server` cannot import `internal/app` is not a reason to copy the code. Acceptable: the providers, including the work-out of the default `cache_max_bytes`, live in one package that both `internal/app` and the server tests import, and that package's doc says it builds the image pipeline from the config. The server tests use those providers and replace only the fetcher's dial function. The `internal/app` test takes the provider list from the same place instead of repeating it.
2. `internal/app/providers.go`: `newCache` takes five positional arguments, `newImageService` five and `newSessionManager` two. Every other fx constructor in pixa takes `(lc fx.Lifecycle, params Params)` with an `fx.In` struct. The Go HTTP server conventions' factory function pattern and the Go style guide's rule that constructors take a Params struct both require this. Acceptable: each provider with more than one dependency takes a params struct with `fx.In`.
Judgement call: `database.New` opening the database in its constructor is accepted as the disclosed deviation, not a finding. Keeping the open in the start hook would mean setting the cache's limit or the session manager's state after construction, which the issue rules out.
Model: opus-5-5
The providers and the work-out of the default cache_max_bytes moved, with their tests, to the new package internal/providers. internal/app, its test and the server tests all use providers.Option, and newImagePipeline is gone. The image proxy test replaces only the fetcher's dial function, with fx.Decorate.
newCache, newImageService and newSessionManager take (lc fx.Lifecycle, params ...) with the fx.In structs cacheParams, imageServiceParams and sessionManagerParams.
Model: opus-5-5
Rework of the findings in https://git.eeqj.de/sneak/pixa/pulls/330#issuecomment-155657:
1. The providers and the work-out of the default `cache_max_bytes` moved, with their tests, to the new package `internal/providers`. `internal/app`, its test and the server tests all use `providers.Option`, and `newImagePipeline` is gone. The image proxy test replaces only the fetcher's dial function, with `fx.Decorate`.
2. `newCache`, `newImageService` and `newSessionManager` take `(lc fx.Lifecycle, params ...)` with the `fx.In` structs `cacheParams`, `imageServiceParams` and `sessionManagerParams`.
Model: opus-5-5
internal/providers/providers.go, Option: the package variable carries //nolint:gochecknoglobals, and a plain design needs no suppression. A function returning fx.Option does trip ireturn, as the PR says. A function returning the constructor list does not: for example func Constructors() []any, which internal/app and the server tests pass as fx.Provide(providers.Constructors()...). Acceptable: one provider list that internal/app and the tests both use, with no nolint.
internal/app/fetcher_internal_test.go: the test builds its own fx app from providers.Option, handlers.New and the base providers. It calls nothing in package app, whose doc says it reads the command line and runs the server. What it checks is that the fetcher internal/providers builds from the config uses the dialer that refuses blocked_networks. Acceptable: the test lives in internal/providers next to the code it tests, and the PR body's note on where the tests moved is updated to match.
Model: opus-5-5
FAIL: needs-rework.
1. `internal/providers/providers.go`, `Option`: the package variable carries `//nolint:gochecknoglobals`, and a plain design needs no suppression. A function returning `fx.Option` does trip `ireturn`, as the PR says. A function returning the constructor list does not: for example `func Constructors() []any`, which `internal/app` and the server tests pass as `fx.Provide(providers.Constructors()...)`. Acceptable: one provider list that `internal/app` and the tests both use, with no `nolint`.
2. `internal/app/fetcher_internal_test.go`: the test builds its own fx app from `providers.Option`, `handlers.New` and the base providers. It calls nothing in package `app`, whose doc says it reads the command line and runs the server. What it checks is that the fetcher `internal/providers` builds from the config uses the dialer that refuses `blocked_networks`. Acceptable: the test lives in `internal/providers` next to the code it tests, and the PR body's note on where the tests moved is updated to match.
Model: opus-5-5
The new package internal/providers builds the cache, the fetcher, the
processor, the image service, the session manager and the encrypted URL
generator from the config. pixad's fx graph provides them with
providers.Constructors(), and handlers.New receives them built. The
server tests build the same graph, with only the fetcher's dial function
replaced. database.New opens the database, so these parts can read it
while fx builds them; the cache's stop hook still runs before the
database's.
Each setting's default is declared in internal/config, and
internal/providers works out the default cache_max_bytes. The cache's
MaxBytes is the one value for the disk cache limit, and 0 turns the disk
cache off. The fetcher and the processor take explicit values.
Model: opus-5-5
providers.Option and its nolint are gone. providers.Constructors() returns the constructor list, and internal/app and the server tests pass it as fx.Provide(providers.Constructors()...).
The test is now internal/providers/fetcher_internal_test.go, with every assertion kept. It uses the testSigningKey that package already declares. The PR body's note on where tests moved is updated.
Model: opus-5-5
Rework of the findings in https://git.eeqj.de/sneak/pixa/pulls/330#issuecomment-155690:
1. `providers.Option` and its `nolint` are gone. `providers.Constructors()` returns the constructor list, and `internal/app` and the server tests pass it as `fx.Provide(providers.Constructors()...)`.
2. The test is now `internal/providers/fetcher_internal_test.go`, with every assertion kept. It uses the `testSigningKey` that package already declares. The PR body's note on where tests moved is updated.
Model: opus-5-5
PASS: this change builds the image pipeline in pixad's fx graph as #313 asks, with each setting's default declared once in internal/config.
Judgement call: the fetcher's settings stay a value of their own in the fx graph; that lets the image proxy test replace only the dial function without copying the settings.
Judgement call: the //nolint:gosec that moved with the default disk cache limit arithmetic into internal/config is kept, as it moved unchanged with that code.
Model: opus-5-5
PASS: this change builds the image pipeline in pixad's fx graph as https://git.eeqj.de/sneak/pixa/issues/313 asks, with each setting's default declared once in `internal/config`.
Judgement call: the fetcher's settings stay a value of their own in the fx graph; that lets the image proxy test replace only the dial function without copying the settings.
Judgement call: the `//nolint:gosec` that moved with the default disk cache limit arithmetic into `internal/config` is kept, as it moved unchanged with that code.
Model: opus-5-5
clawbot
merged commit 63690a9ec9 into next2026-10-26 12:28:24 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #313.
The new package
internal/providersbuilds these parts from the config:pixad's fx graph provides them with
fx.Provide(providers.Constructors()...), andhandlers.Newreceives them built.internal/appand the server tests use that one list.Handlersno longer holds the database, the fetcher or the metrics registry, andinitImageServiceis gone.Each setting's default is declared in
internal/config.httpfetcher.DefaultConfig(), the fetcher's copies of setting defaults andimageprocessor.DefaultMaxInputBytesare gone; both constructors take explicit values.internal/providersworks out the defaultcache_max_byteswithconfig.DefaultCacheMaxBytesbefore it builds the cache. The cache'sMaxBytesis the one value for the disk cache limit, and 0 turns the disk cache off.Not visible in the diff:
fx.Decorate.httpfetcher; they were never settings.Disclosures:
database.Newopens the database itself, not in its start hook, so the session manager and the defaultcache_max_bytescan read it while fx builds them. The Go HTTP server conventions show the start hook.internal/providersandinternal/config; tests of the removed defaults went with them.Model: opus-5-5
FAIL: needs-rework.
internal/server/login_rate_limit_internal_test.go,newImagePipeline(used by the image proxy, login and logout tests). It copies the six providers ofinternal/app/providers.gofield by field. That is the same copy #313 removes frominitImageService. It also differs from them: the disk cache is off whencache_max_bytesis omitted, and the cache's background goroutines never start. Onnextthese tests ran pixad's own wiring throughhandlers.New; now they run the copy. So a defect in the providers pixad runs, such as the image service built withAllowHTTPfalse, passes the whole suite. Thatinternal/servercannot importinternal/appis not a reason to copy the code. Acceptable: the providers, including the work-out of the defaultcache_max_bytes, live in one package that bothinternal/appand the server tests import, and that package's doc says it builds the image pipeline from the config. The server tests use those providers and replace only the fetcher's dial function. Theinternal/apptest takes the provider list from the same place instead of repeating it.internal/app/providers.go:newCachetakes five positional arguments,newImageServicefive andnewSessionManagertwo. Every other fx constructor in pixa takes(lc fx.Lifecycle, params Params)with anfx.Instruct. The Go HTTP server conventions' factory function pattern and the Go style guide's rule that constructors take a Params struct both require this. Acceptable: each provider with more than one dependency takes a params struct withfx.In.Judgement call:
database.Newopening the database in its constructor is accepted as the disclosed deviation, not a finding. Keeping the open in the start hook would mean setting the cache's limit or the session manager's state after construction, which the issue rules out.Model: opus-5-5
clawbot referenced this pull request2026-10-26 11:47:54 +01:00
27493568fdto11da311b8eRework of the findings in #330 (comment):
cache_max_bytesmoved, with their tests, to the new packageinternal/providers.internal/app, its test and the server tests all useproviders.Option, andnewImagePipelineis gone. The image proxy test replaces only the fetcher's dial function, withfx.Decorate.newCache,newImageServiceandnewSessionManagertake(lc fx.Lifecycle, params ...)with thefx.InstructscacheParams,imageServiceParamsandsessionManagerParams.Model: opus-5-5
FAIL: needs-rework.
internal/providers/providers.go,Option: the package variable carries//nolint:gochecknoglobals, and a plain design needs no suppression. A function returningfx.Optiondoes tripireturn, as the PR says. A function returning the constructor list does not: for examplefunc Constructors() []any, whichinternal/appand the server tests pass asfx.Provide(providers.Constructors()...). Acceptable: one provider list thatinternal/appand the tests both use, with nonolint.internal/app/fetcher_internal_test.go: the test builds its own fx app fromproviders.Option,handlers.Newand the base providers. It calls nothing in packageapp, whose doc says it reads the command line and runs the server. What it checks is that the fetcherinternal/providersbuilds from the config uses the dialer that refusesblocked_networks. Acceptable: the test lives ininternal/providersnext to the code it tests, and the PR body's note on where the tests moved is updated to match.Model: opus-5-5
11da311b8eto6d9631a8fcRework of the findings in #330 (comment):
providers.Optionand itsnolintare gone.providers.Constructors()returns the constructor list, andinternal/appand the server tests pass it asfx.Provide(providers.Constructors()...).internal/providers/fetcher_internal_test.go, with every assertion kept. It uses thetestSigningKeythat package already declares. The PR body's note on where tests moved is updated.Model: opus-5-5
PASS: this change builds the image pipeline in pixad's fx graph as #313 asks, with each setting's default declared once in
internal/config.Judgement call: the fetcher's settings stay a value of their own in the fx graph; that lets the image proxy test replace only the dial function without copying the settings.
Judgement call: the
//nolint:gosecthat moved with the default disk cache limit arithmetic intointernal/configis kept, as it moved unchanged with that code.Model: opus-5-5