Conversation
- Makefile: deadcode/deadcode-backend/deadcode-frontend targets. Backend uses golangci-lint (already wired) + x/tools/deadcode for reachability from ./cmd entrypoints. Frontend uses knip + depcheck via bunx. - lint-backend now finds golangci-lint at $(go env GOPATH)/bin when not in PATH. - .gitignore: replace .dagrobin/db with .dagrobin/ to also cover backend/ and frontend/ subdir DBs; add .rtk/ for local rtk filters.
Backend (deadcode/reachability from ./cmd): - pkg/logger: drop unused Logger.Debug method. - pkg/middleware: drop RequireAnyFeature handler and its orphan tests (no production caller; tests were the only references). - pkg/middleware: drop unused KeyByUserID rate-limit key fn. Frontend (knip + depcheck): - src/db/index.ts: remove unused Dexie/IndexedDB layer (leftover from the RAG removal); drop dexie dependency. - src/services/featureFlags.ts: drop unused invalidateFeatureFlags and getFlags exports. - package.json: remove unused @vueuse/core and @vue/test-utils. Verified: rtk go build/vet/test ./... and frontend vue-tsc + vitest all pass; make deadcode is clean on backend (frontend has 2 known false-positives: env.d.ts referenced via tsconfig, @types/qrcode is the types peer for qrcode).
Layer-2 sweep (deadcode + endpoint-vs-frontend reachability + schema
audit). All decisions logged in .claude/IMPROVEMENTS.md.
Removed (dead in practice):
- internal/testutil: MockWaitlistRepo struct + 4 methods (zero usage).
- internal/handlers/auth.go: VerifyEmail handler and its route in
cmd/server/main.go. The handler read a verify:<token> Redis key
that nothing in the project ever wrote (no email-sender), and
registration auto-verifies. Half-built feature with no path to
completion until SMTP integration lands. email_verified column
retained for when it's rebuilt.
- internal/handlers/blog.go: AdminUploadCover (cover-specific upload
that required the post to already exist; never called).
Fixed (live bug surfaced by the sweep):
- AdminBlog.vue uploadCover() was POSTing to /api/v1/admin/upload,
which had no handler — cover uploads 404'd silently. Replaced
AdminUploadCover with a generic AdminHandler.Upload that accepts a
prefix form field (allowlisted) and returns {url}, matching the
upload-before-create pattern the frontend uses. Frontend now sends
prefix=covers.
Documented (deferred):
- .claude/IMPROVEMENTS.md captures the email-verification rebuild
path, the ForgotPassword/ResetPassword placeholders that pretend
success, the orphan health_checks table (migration 008 with no Go
reader/writer), and the handler test-coverage gaps.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR consolidates upload endpoints and removes dead code identified by a dead-code sweep. The main change replaces a blog-specific cover upload handler with a generic admin upload endpoint accepting an allowlisted prefix form field. Related cleanup removes email verification handlers, unused middleware/service methods, orphaned test doubles, and obsolete frontend dependencies. ChangesDead Code Removal & Upload Endpoint Consolidation
Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant Router
participant AdminHandler
participant Storage
AdminUI->>Router: POST /api/v1/admin/upload (multipart: file + prefix)
Router->>AdminHandler: Upload(ctx)
AdminHandler->>Storage: UploadFormFile(ctx, file, prefix)
Storage-->>AdminHandler: url / error
AdminHandler-->>AdminUI: { "url": url } or 4xx/5xx
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/views/Admin/AdminBlog.vue (1)
235-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
uploadCoversilently swallows server-side errors — checkr.okbefore parsing.
fetchonly rejects on network failures; HTTP 4xx/5xx responses still resolve. Without anr.okguard, a rejected upload (e.g. auth failure, oversized file, invalid prefix) returns{"error":"..."},data.urlisundefined,form.value.cover_imageis set toundefined, and the user sees no error message.🐛 Proposed fix
- ).then(r => r.json()) as { url: string } - form.value.cover_image = data.url + ) + if (!r.ok) { + const errBody = await r.json().catch(() => ({})) as { error?: string } + throw new Error(errBody.error || `Upload failed (${r.status})`) + } + const data = await r.json() as { url: string } + form.value.cover_image = data.url🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/Admin/AdminBlog.vue` around lines 235 - 244, The uploadCover implementation currently parses the response without checking HTTP status, which can set form.value.cover_image to undefined on 4xx/5xx; update the upload logic in the uploadCover handler to check the fetch Response.ok before using the body: await fetch(...) -> const r = await fetch(...); const data = await r.json(); if (!r.ok) { formError.value = data.error || `Upload failed: ${r.status} ${r.statusText}`; return; } form.value.cover_image = data.url; keep the existing catch to handle network errors. Ensure you reference the uploadCover handler and the variables form, formError, and cover_image when applying this change.
🧹 Nitpick comments (3)
backend/cmd/server/main.go (1)
354-354: 💤 Low valueMove the
/uploadroute out of the "Blog — admin routes" block.
adminHandler.Uploadis the generic multi-prefix upload endpoint, not blog-specific. Placing it inside the blog comment block will confuse readers. Consider putting it in a dedicated "Admin — generic" section or directly before the blog routes block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/cmd/server/main.go` at line 354, The route admin.Post("/upload", adminHandler.Upload) is incorrectly placed in the "Blog — admin routes" block; move this generic upload endpoint out of that blog-specific section and place it in a dedicated "Admin — generic" section (or immediately before the blog routes) so readers see adminHandler.Upload as a global admin endpoint rather than blog-specific.backend/internal/handlers/admin.go (1)
54-61: 💤 Low valueMove
allowedto a package-level variable.The map literal is reallocated on every request. Declaring it once as a
varat package level is idiomatic Go and avoids repeated allocation.♻️ Proposed refactor
+var allowedUploadPrefixes = map[string]bool{ + "uploads": true, + "covers": true, + "banners": true, + "linktree": true, + "brand-kit": true, + "products": true, +} + func (h *AdminHandler) Upload(c *fiber.Ctx) error { file, err := c.FormFile("file") if err != nil { return fiber.NewError(fiber.StatusBadRequest, "file is required") } prefix := c.FormValue("prefix", "uploads") - allowed := map[string]bool{ - "uploads": true, - "covers": true, - "banners": true, - "linktree": true, - "brand-kit": true, - "products": true, - } - if !allowed[prefix] { + if !allowedUploadPrefixes[prefix] { return fiber.NewError(fiber.StatusBadRequest, "invalid prefix") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/handlers/admin.go` around lines 54 - 61, Move the local "allowed" map literal to a package-level variable to avoid reallocating it on every request: declare a package-scope var like `var allowed = map[string]bool{ "uploads": true, "covers": true, "banners": true, "linktree": true, "brand-kit": true, "products": true }` at the top of the file, then remove the local `allowed := map[string]bool{...}` in the handler and use the package-level `allowed` map directly (refer to the existing usage of `allowed` in the handler where it is currently defined).Makefile (1)
134-134: 💤 Low valuePin
deadcodeto a specific version instead of@latest.
go run golang.org/x/tools/cmd/deadcode@latestre-downloads and potentially upgrades the tool on every invocation. Different team members or CI runs could see different results across versions. Pin to a known-good release (e.g.@v0.44.0, the current stable version).♻️ Proposed fix
- cd backend && go run golang.org/x/tools/cmd/deadcode@latest ./cmd/... + cd backend && go run golang.org/x/tools/cmd/deadcode@v0.44.0 ./cmd/...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 134, Replace the use of the floating `@latest` version for the deadcode tool in the Makefile command `go run golang.org/x/tools/cmd/deadcode@latest ./cmd/...` with a pinned release (for example `@v0.44.0`) so the tool invocation is deterministic across developers and CI; update the `go run golang.org/x/tools/cmd/deadcode@latest` reference to use the chosen semantic version tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/IMPROVEMENTS.md:
- Around line 36-48: AuthHandler.ResetPassword in auth.go currently returns a
successful response without changing any password; change the handler to stop
returning 200-success and instead return a clear 501 Not Implemented (or
equivalent error) response and log a descriptive message so the UI does not
falsely report success. Locate the ResetPassword method (and similarly
ForgotPassword if it currently no-ops) and replace the silent success path with
a short-circuit that returns the 501 response and an explanatory log entry;
leave the routes in place so UI links don’t break and add a TODO comment
referencing the future work: email-sender prerequisite, token storage, and
actual password update flow.
---
Outside diff comments:
In `@frontend/src/views/Admin/AdminBlog.vue`:
- Around line 235-244: The uploadCover implementation currently parses the
response without checking HTTP status, which can set form.value.cover_image to
undefined on 4xx/5xx; update the upload logic in the uploadCover handler to
check the fetch Response.ok before using the body: await fetch(...) -> const r =
await fetch(...); const data = await r.json(); if (!r.ok) { formError.value =
data.error || `Upload failed: ${r.status} ${r.statusText}`; return; }
form.value.cover_image = data.url; keep the existing catch to handle network
errors. Ensure you reference the uploadCover handler and the variables form,
formError, and cover_image when applying this change.
---
Nitpick comments:
In `@backend/cmd/server/main.go`:
- Line 354: The route admin.Post("/upload", adminHandler.Upload) is incorrectly
placed in the "Blog — admin routes" block; move this generic upload endpoint out
of that blog-specific section and place it in a dedicated "Admin — generic"
section (or immediately before the blog routes) so readers see
adminHandler.Upload as a global admin endpoint rather than blog-specific.
In `@backend/internal/handlers/admin.go`:
- Around line 54-61: Move the local "allowed" map literal to a package-level
variable to avoid reallocating it on every request: declare a package-scope var
like `var allowed = map[string]bool{ "uploads": true, "covers": true, "banners":
true, "linktree": true, "brand-kit": true, "products": true }` at the top of the
file, then remove the local `allowed := map[string]bool{...}` in the handler and
use the package-level `allowed` map directly (refer to the existing usage of
`allowed` in the handler where it is currently defined).
In `@Makefile`:
- Line 134: Replace the use of the floating `@latest` version for the deadcode
tool in the Makefile command `go run golang.org/x/tools/cmd/deadcode@latest
./cmd/...` with a pinned release (for example `@v0.44.0`) so the tool invocation
is deterministic across developers and CI; update the `go run
golang.org/x/tools/cmd/deadcode@latest` reference to use the chosen semantic
version tag.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15b416c1-e262-444e-b4e3-ff55b383a308
⛔ Files ignored due to path filters (1)
frontend/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.claude/IMPROVEMENTS.md.gitignoreMakefilebackend/cmd/server/main.gobackend/internal/handlers/admin.gobackend/internal/handlers/auth.gobackend/internal/handlers/blog.gobackend/internal/testutil/mocks.gobackend/pkg/logger/logger.gobackend/pkg/middleware/feature_flags.gobackend/pkg/middleware/middleware_test.gobackend/pkg/middleware/ratelimit.gofrontend/package.jsonfrontend/src/db/index.tsfrontend/src/services/featureFlags.tsfrontend/src/views/Admin/AdminBlog.vue
💤 Files with no reviewable changes (10)
- backend/internal/handlers/blog.go
- backend/pkg/middleware/feature_flags.go
- frontend/src/services/featureFlags.ts
- frontend/src/db/index.ts
- backend/internal/testutil/mocks.go
- backend/pkg/logger/logger.go
- backend/pkg/middleware/ratelimit.go
- backend/internal/handlers/auth.go
- frontend/package.json
- backend/pkg/middleware/middleware_test.go
Comment on lines
+36
to
+48
| ## Password reset — placeholder, kept | ||
|
|
||
| `AuthHandler.ResetPassword` (auth.go) returns success **without actually | ||
| resetting any password**. It's a security smell: the UI thinks the reset | ||
| worked, but the password is unchanged. `ForgotPassword` is also a no-op (good | ||
| fail-silent UX, but no email is sent). | ||
|
|
||
| Not removed because the routes/UI exist and removing them would visibly break | ||
| the "forgot password" link. Keep until a real reset flow is built. | ||
|
|
||
| **To fix:** same email-sender prerequisite as email verification, plus token | ||
| storage and password update in the handler. | ||
|
|
Contributor
There was a problem hiding this comment.
ResetPassword silently succeeds without changing the password — prioritize fixing this.
Leaving a route registered that reports success to the client while performing a no-op password change is a significant security UX defect: users who think they've changed their password haven't, and may be locked out or falsely confident their account is secured. This is more urgent than most of the other deferred items and shouldn't wait for the full email-sender implementation — a simple 501 / "not yet implemented" error response is safer than a false 200.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/IMPROVEMENTS.md around lines 36 - 48, AuthHandler.ResetPassword in
auth.go currently returns a successful response without changing any password;
change the handler to stop returning 200-success and instead return a clear 501
Not Implemented (or equivalent error) response and log a descriptive message so
the UI does not falsely report success. Locate the ResetPassword method (and
similarly ForgotPassword if it currently no-ops) and replace the silent success
path with a short-circuit that returns the 501 response and an explanatory log
entry; leave the routes in place so UI links don’t break and add a TODO comment
referencing the future work: email-sender prerequisite, token storage, and
actual password update flow.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: afa7789 <26887703+afa7789@users.noreply.github.com>
Contributor
Resolved and pushed in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/handlers/admin.go (1)
33-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against nil
storagedependency before serving uploads
h.storageis required forUpload, butNewAdminHandleraccepts it without validation. A nil dependency here can cause runtime failure on the upload path. Please fail fast in constructor (or short-circuit inUpload) whenstorageis nil.Suggested fix
func NewAdminHandler( @@ storage domain.Storage, cfg *config.Config, ) *AdminHandler { + if storage == nil { + panic("handlers.NewAdminHandler: storage dependency is nil") + } return &AdminHandler{ @@ } }Also applies to: 71-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/handlers/admin.go` around lines 33 - 45, NewAdminHandler must fail fast when the storage dependency is nil to avoid runtime panics in Upload: update NewAdminHandler to validate the storage parameter (and other required deps) and return an error (change signature to (*AdminHandler, error)) or explicitly panic with a clear message if storage == nil; additionally add a defensive nil-check at the start of AdminHandler.Upload (check h.storage) to return a clear error rather than dereferencing nil. Target the NewAdminHandler constructor and the Upload method (references: NewAdminHandler, AdminHandler, Upload, h.storage) and ensure any call sites are updated to handle the constructor's error return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/handlers/admin.go`:
- Line 76: The log statement in admin.Upload currently logs the
client-controlled file.Filename raw, which risks log-forging and sensitive data
leakage; update the admin.Upload logging to avoid raw filenames by replacing
file.Filename with a sanitized or quoted representation (e.g., use a
quoting/sanitization helper or format specifier like %q) and/or omit the
filename entirely; locate the log.Printf call referencing prefix and
file.Filename and change it to use the sanitized variable (create a small
sanitizeFilename function if needed) before logging.
---
Outside diff comments:
In `@backend/internal/handlers/admin.go`:
- Around line 33-45: NewAdminHandler must fail fast when the storage dependency
is nil to avoid runtime panics in Upload: update NewAdminHandler to validate the
storage parameter (and other required deps) and return an error (change
signature to (*AdminHandler, error)) or explicitly panic with a clear message if
storage == nil; additionally add a defensive nil-check at the start of
AdminHandler.Upload (check h.storage) to return a clear error rather than
dereferencing nil. Target the NewAdminHandler constructor and the Upload method
(references: NewAdminHandler, AdminHandler, Upload, h.storage) and ensure any
call sites are updated to handle the constructor's error return.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dfe3dc7-5788-4062-aa09-c4b5862d529c
📒 Files selected for processing (3)
backend/cmd/server/main.gobackend/internal/handlers/admin.gobackend/internal/handlers/blog.go
💤 Files with no reviewable changes (1)
- backend/internal/handlers/blog.go
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/cmd/server/main.go
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes / Changes
Documentation
Chores