refactor: cut dead code, redundant parsers, and unused frontend deps - #252
Open
s0up4200 wants to merge 5 commits into
Open
refactor: cut dead code, redundant parsers, and unused frontend deps#252s0up4200 wants to merge 5 commits into
s0up4200 wants to merge 5 commits into
Conversation
Repo-wide over-engineering audit. No behavior change. Backend: delete the unused internal/broadcaster package and its pass-through wrapper (the real wiring is a plain func value), the dead TestRunner/ProgressBroadcaster/UserService interfaces, unused query helpers, and a duplicate getMigrationVersion. Collapse the tailscale Client interface and its two identical wrapper structs into a type alias for local.Client, which they both already wrapped. Drop the redundant batch traceroute parsers: there is one exec path and it uses the streaming parser, whose parseHopLine already handles Windows and carries its own IPv6 test. Remove seven config knobs that were parsed, defaulted and written to the generated TOML but never read, along with their README entries. Unknown keys are ignored on decode, so existing config files keep working. Swap the hand-rolled Tailscale CIDR check for tsaddr.IsTailscaleIP from the tailscale dep already in the tree, and GenerateSecureToken for stdlib crypto/rand.Text, which drops four error branches. The session secret concatenates two calls to keep its previous ~256 bits. Frontend: drop @mui/material and its two emotion peers, used for a single Container that Tailwind classes replace exactly; @headlessui/react, which had no imports; and four @FortAwesome packages covering six icons that react-icons already provides. Bundle drops 164 kB (50 kB gzipped).
📝 WalkthroughWalkthroughThe PR reduces obsolete Go and frontend APIs, adds update-checking and licensing/theme server wiring, streams traceroute parsing, revises Tailscale and ntfy behavior, simplifies migration configuration, updates frontend icon and theme handling, and refreshes configuration documentation. ChangesNetronome cleanup
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Follow-up to the audit. Whole-module deadcode analysis reported these as unreachable from any entry point including tests, and each was confirmed by grep to have no caller. Dropped auth.HashPassword and auth.CheckPassword, dead duplicates of the bcrypt path that database/user.go actually uses; OIDCConfig.AuthURL, superseded by AuthURLWithPKCE; config.GetDefaultConfigPath, which DefaultConfigPaths replaced; migrations.ReadMigration; notifications.MigrateDiscordWebhook; Server.SetPacketLossService; web.BuildFrontend, which shelled out to pnpm and has no caller in Go, the Makefile or CI; and two unused test helpers. pkg/migrator loses its unreachable API surface: the WithTableName, WithSchemaString and WithSchemaFile options, LoggerFunc, Migration.Id, TableDrop, Exec, BeginTx, Pending, and migrateInitialSchemaOpt. Removing the schema options makes the initialSchema and initialSchemaFile fields dead, so those go too. tableName stays; it still has a default and is read when building the version table. Finally, migrations.parseInt was a hand-rolled strconv.Atoi. The two agree on every input reachable here, including the empty string, which both resolve to a zero version.
Third audit pass. Each type below appeared exactly twice in the tree, as its own declaration and doc comment, with no other reference: types.MonitorFullData, an 85-line vnstat export struct the handler never used because it builds a map by hand; types.MonitorBandwidth, which describes a table no migration creates; speedtest.IperfResult, superseded by iperfEndData; and notifications.PacketLossNotification. web/src/api/tailscale.ts had no importers. The packetloss history cutover plan under docs/ shipped with 20 unticked boxes and absolute paths from a local worktree; migration 021 shows the work landed. scripts/check_vnstat_data.sh is referenced from nothing. .golangci.yml pointed local-prefixes at github.com/autobrr/qui, so import grouping never applied to this project.
Fourth audit pass, frontend side. The repo's knip.json turns off export checking and ignores api/, utils/, types/ and constants/, so none of this surfaces normally; it was found with a temporary config and then each symbol was checked by hand. Deleted 17 exports and types that had no reference anywhere: getOIDCLoginUrl, getMonitorAgent, getThresholdOperatorLabel, getEventCategoryIcon, SPRING_TRANSITION, SERVER_DISPLAY_INCREMENT, getRTTColorClass, formatHopData, toggleDarkMode, hasManualPreference, formatDate, convertUTCToTimezone, convertTimezoneToUTC, formatUTCInTimezone, MonitorRefreshInterval, SpeedTestHistory and SpeedUpdate. formatDate and SPRING_TRANSITION looked used but are shadowed by separate local definitions in MonitorResultsTable and TabNavigation. Six more were exported yet only ever called inside their own file, so they lose the export instead of being removed: convertIperfServersToServerFormat, filterServers, sortServers, flagEmojis, resetToSystemTheme and getEffectiveTimezone. Dropping the last of the timezone converters left toZonedTime unused, so it comes off the date-fns-tz import; the package stays for fromZonedTime and formatInTimeZone. pwa.d.ts declared virtual:pwa-register/react, which is never imported because the app uses workbox-window directly; service-worker.d.ts declared two interfaces nothing referenced. The EventRuleItem re-export in the notifications barrel was unused, as the one consumer imports the component directly. Verified: tsc clean, built CSS byte-identical, lint unchanged at 25.
s0up4200
marked this pull request as ready for review
July 28, 2026 18:54
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
README.md (1)
784-787: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument removed configuration in
docs/.Add a migration note listing removed variables and their replacement or removal behavior; README updates alone do not meet the repository requirement.
As per coding guidelines, document configuration changes in
docs/when behavior changes.Also applies to: 800-802, 819-820, 845-848
🤖 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 `@README.md` around lines 784 - 787, Document the removed configuration variables shown in the README diff in the appropriate docs migration note, including each variable’s replacement or explicit removal behavior. Keep the README changes, but add the required documentation under docs/ so all affected settings are covered.Source: Coding guidelines
pkg/migrator/migrator.go (2)
222-245: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the base migration and version record atomic.
migration.Run(m.db)executes outsidetx, while the version row is written inside it. If version recording or commit fails, the schema change can persist without its migration record, causing the base migration to be attempted again. Require a transactionalRunTxfor the initial migration, or redesignRunto receive the active transaction.🤖 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 `@pkg/migrator/migrator.go` around lines 222 - 245, Make the initial migration path atomic with its schema-version update by removing or disallowing the non-transactional migration.Run(m.db) branch for the base migration. Require migration.RunTx(tx), or change the migration.Run contract to receive and use the active transaction, while preserving the existing transactional updateSchemaVersion flow.
190-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate initial-schema commit failures.
This function has an unnamed return value, so
return errevaluates before the deferredtx.Commit(). A commit failure is assigned only to the localerrand the caller receivesnil, reporting a failed base migration as successful. Use a namederrresult and apply the same correction tomigrate.Proposed fix
-func (m *Migrator) migrateInitialSchema(migration *Migration) error { +func (m *Migrator) migrateInitialSchema(migration *Migration) (err error) {🤖 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 `@pkg/migrator/migrator.go` around lines 190 - 218, Update migrateInitialSchema and migrate to use named error return values so deferred transaction commit failures are propagated to callers. Preserve the existing rollback and commit logic, ensuring the deferred assignment to err is returned when either migration function completes.
🤖 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 `@web/src/components/Footer.tsx`:
- Line 31: Preserve decorative SVG accessibility by adding aria-hidden="true" to
FaDiscord and SiReadme in web/src/components/Footer.tsx (lines 31 and 51), the
Apple/Linux icons in web/src/components/monitor/MonitorSystemInfo.tsx (lines
85-87), the OS icon returned by getOSIcon() and mobile kernel icons in
web/src/components/monitor/tabs/MonitorOverviewTab.tsx (lines 292-295 and
484-486), and the returned laptop icon abstraction in
web/src/utils/agentIcons.tsx (line 37).
---
Outside diff comments:
In `@pkg/migrator/migrator.go`:
- Around line 222-245: Make the initial migration path atomic with its
schema-version update by removing or disallowing the non-transactional
migration.Run(m.db) branch for the base migration. Require migration.RunTx(tx),
or change the migration.Run contract to receive and use the active transaction,
while preserving the existing transactional updateSchemaVersion flow.
- Around line 190-218: Update migrateInitialSchema and migrate to use named
error return values so deferred transaction commit failures are propagated to
callers. Preserve the existing rollback and commit logic, ensuring the deferred
assignment to err is returned when either migration function completes.
In `@README.md`:
- Around line 784-787: Document the removed configuration variables shown in the
README diff in the appropriate docs migration note, including each variable’s
replacement or explicit removal behavior. Keep the README changes, but add the
required documentation under docs/ so all affected settings are covered.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f56df75-4514-48b8-bac5-d785f2d5c27b
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
.golangci.ymlREADME.mdconfig/config.tomldocs/superpowers/plans/2026-04-09-packetloss-history-cutover.mdinternal/auth/auth.gointernal/auth/oidc.gointernal/broadcaster/broadcaster.gointernal/config/config.gointernal/database/database.gointernal/database/database_test.gointernal/database/migrations/migrations.gointernal/database/user.gointernal/monitor/tailscale_discovery.gointernal/notifications/notifications.gointernal/server/auth.gointernal/server/auth_oidc.gointernal/server/server.gointernal/speedtest/iperf.gointernal/speedtest/progress_broadcaster.gointernal/speedtest/traceroute.gointernal/speedtest/traceroute_test.gointernal/speedtest/types.gointernal/tailscale/tailscale.gointernal/types/types.gointernal/utils/crypto.gointernal/utils/tailscale.gopkg/migrator/migrator.goscripts/check_vnstat_data.shweb/build.goweb/package.jsonweb/src/api/auth.tsweb/src/api/monitor.tsweb/src/api/notifications.tsweb/src/api/tailscale.tsweb/src/components/Footer.tsxweb/src/components/Main.tsxweb/src/components/auth/Login.tsxweb/src/components/monitor/MonitorSystemInfo.tsxweb/src/components/monitor/tabs/MonitorOverviewTab.tsxweb/src/components/settings/notifications/index.tsweb/src/components/speedtest/traceroute/constants/tracerouteConstants.tsweb/src/components/speedtest/traceroute/utils/serverUtils.tsweb/src/components/speedtest/traceroute/utils/tracerouteUtils.tsweb/src/constants/monitorRefreshIntervals.tsweb/src/types/pwa.d.tsweb/src/types/service-worker.d.tsweb/src/types/speedtest.tsweb/src/types/types.tsweb/src/utils/agentIcons.tsxweb/src/utils/countryFlags.tsweb/src/utils/darkMode.tsweb/src/utils/timeSettings.ts
💤 Files with no reviewable changes (28)
- config/config.toml
- scripts/check_vnstat_data.sh
- internal/broadcaster/broadcaster.go
- internal/speedtest/progress_broadcaster.go
- web/src/types/pwa.d.ts
- internal/utils/crypto.go
- web/src/api/tailscale.ts
- docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md
- internal/auth/oidc.go
- web/src/api/auth.ts
- web/src/types/service-worker.d.ts
- web/src/api/monitor.ts
- web/package.json
- web/src/types/speedtest.ts
- web/src/api/notifications.ts
- web/build.go
- web/src/types/types.ts
- internal/speedtest/iperf.go
- internal/server/server.go
- internal/speedtest/types.go
- internal/database/database_test.go
- internal/types/types.go
- web/src/components/speedtest/traceroute/utils/tracerouteUtils.ts
- internal/database/database.go
- web/src/components/speedtest/traceroute/constants/tracerouteConstants.ts
- internal/speedtest/traceroute_test.go
- internal/speedtest/traceroute.go
- internal/notifications/notifications.go
| }} | ||
| > | ||
| <FontAwesomeIcon icon={faDiscord} className="h-4 w-4" /> | ||
| <FaDiscord className="h-4 w-4" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== affected files =="
git ls-files | rg '^web/src/(components/Footer\.tsx|components/monitor/MonitorSystemInfo\.tsx|components/monitor/tabs/MonitorOverviewTab\.tsx|utils/agentIcons\.tsx)$' || true
echo
echo "== package versions =="
for f in package.json web/package.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
node -e "const p=require('./$f'); console.log(JSON.stringify({deps:p.dependencies, devDeps:p.devDependencies}, null, 2))" | rg -i 'react-icons|fontawesome|font-awesome|tailwind|vite|eslint' -C 1 || true
fi
done
echo
echo "== imports/usages in affected files =="
for f in web/src/components/Footer.tsx web/src/components/monitor/MonitorSystemInfo.tsx web/src/components/monitor/tabs/MonitorOverviewTab.tsx web/src/utils/agentIcons.tsx; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,120p' "$f" | cat -n
fi
done
echo
echo "== react-icons and fontawesome deps in manifests/lockfiles =="
rg -n '"react-icons"|react-icons@|`@fortawesome`|font-awesome"|"react-icons' -S package.json web/package.json pnpm-lock.yaml web/package.json 2>/dev/null || trueRepository: autobrr/netronome
Length of output: 17612
🌐 Web query:
react-icons 5.7.0 iconBase aria-hidden default SVG
💡 Result:
In react-icons version 5.7.0, the IconBase component does not apply a default aria-hidden attribute to the SVG element [1]. The component renders the SVG element without any automatic accessibility attributes by default [1]. To ensure icons are properly hidden from assistive technologies (which is recommended for decorative icons), you must explicitly add the aria-hidden="true" attribute [2]. There are two primary ways to apply this: 1. Per-Icon: You can pass the attribute directly to the icon component: [2] 2. Globally via IconContext: You can use the IconContext.Provider to apply the attribute to all icons within your application or a specific section of your component tree [3][4]: <IconContext.Provider value={{ attr: { 'aria-hidden': 'true' } }}> </IconContext.Provider> [4] The IconBase component merges the attr object from the IconContext with any other props passed to the icon [1][5][4]. If you need to include other SVG-related attributes (such as focusable="false"), you can include them within the same attr object in your context configuration [4][6].
Citations:
- 1: https://github.com/react-icons/react-icons/blob/master/packages/react-icons/src/iconBase.tsx
- 2: https://mintlify.wiki/react-icons/react-icons/guides/accessibility
- 3: https://react-icons-react-icons.mintlify.app/core-concepts/typescript
- 4: https://mintlify.wiki/react-icons/react-icons/core-concepts/icon-context
- 5: https://react-icons-react-icons.mintlify.app/api/icon-component
- 6: https://mintlify.wiki/react-icons/react-icons/api/types/icon-context
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MonitorOverviewTab icon usages =="
sed -n '260,310p' web/src/components/monitor/tabs/MonitorOverviewTab.tsx | cat -n
sed -n '460,495p' web/src/components/monitor/tabs/MonitorOverviewTab.tsx | cat -n
echo
echo "== iconContext usage =="
rg -n "IconContext|IconContext\.Provider|aria-hidden" web/src -SRepository: autobrr/netronome
Length of output: 10119
Preserve decorative SVG accessibility across the icon migration. react-icons@5.7.0 does not add aria-hidden automatically, so these decorative icons should either opt into a shared IconContext attribute or set aria-hidden="true" where used.
web/src/components/Footer.tsx#L31: add decorative SVG attributes toFaDiscord.web/src/components/Footer.tsx#L51: add decorative SVG attributes toSiReadme.web/src/components/monitor/MonitorSystemInfo.tsx#L85-87: addaria-hidden="true"to the Apple/Linux icons.web/src/components/monitor/tabs/MonitorOverviewTab.tsx#L292-295: addaria-hidden="true"to the OS icon returned bygetOSIcon().web/src/components/monitor/tabs/MonitorOverviewTab.tsx#L484-486: addaria-hidden="true"to the mobile kernel icons.web/src/utils/agentIcons.tsx#L37: preserve the attribute in the returned laptop icon abstraction.
📍 Affects 4 files
web/src/components/Footer.tsx#L31-L31(this comment)web/src/components/Footer.tsx#L51-L51web/src/components/monitor/MonitorSystemInfo.tsx#L85-L87web/src/components/monitor/tabs/MonitorOverviewTab.tsx#L292-L295web/src/components/monitor/tabs/MonitorOverviewTab.tsx#L484-L486web/src/utils/agentIcons.tsx#L37-L37
🤖 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 `@web/src/components/Footer.tsx` at line 31, Preserve decorative SVG
accessibility by adding aria-hidden="true" to FaDiscord and SiReadme in
web/src/components/Footer.tsx (lines 31 and 51), the Apple/Linux icons in
web/src/components/monitor/MonitorSystemInfo.tsx (lines 85-87), the OS icon
returned by getOSIcon() and mobile kernel icons in
web/src/components/monitor/tabs/MonitorOverviewTab.tsx (lines 292-295 and
484-486), and the returned laptop icon abstraction in
web/src/utils/agentIcons.tsx (line 37).
Source: MCP tools
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)
internal/config/config.go (1)
112-116: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the removed configuration keys in
docs/.
DEFAULT_PAGE_SIZE,MAX_PAGE_SIZE, the packet-loss defaults,MONITOR_RECONNECT_INTERVAL, andTAILSCALE_AGENT_ACCEPT_ROUTESare no longer accepted by the config parser, and there is no documentation reference for them. Add a migration/reference note underdocs/describing these removals and their ignored behavior, as required for configuration changes.🤖 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 `@internal/config/config.go` around lines 112 - 116, Add a migration/reference note under docs documenting that DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE, the packet-loss defaults, MONITOR_RECONNECT_INTERVAL, and TAILSCALE_AGENT_ACCEPT_ROUTES were removed from the config parser and are now ignored. Keep the note focused on configuration migration behavior and reference the PaginationConfig change where appropriate.Source: Coding guidelines
🤖 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 `@internal/tailscale/tailscale.go`:
- Around line 63-64: Update ListenOnTailscale to build its listener address with
net.JoinHostPort instead of fmt.Sprintf, preserving the selected Tailscale IP
and port for both IPv4 and IPv6. Add table-driven regression tests covering both
address families and verify the function reaches net.Listen with a valid
host-port address.
---
Outside diff comments:
In `@internal/config/config.go`:
- Around line 112-116: Add a migration/reference note under docs documenting
that DEFAULT_PAGE_SIZE, MAX_PAGE_SIZE, the packet-loss defaults,
MONITOR_RECONNECT_INTERVAL, and TAILSCALE_AGENT_ACCEPT_ROUTES were removed from
the config parser and are now ignored. Keep the note focused on configuration
migration behavior and reference the PaginationConfig change where appropriate.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 53a9bfa1-9b01-41e7-bc18-ee419de2041c
⛔ Files ignored due to path filters (1)
web/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
.golangci.ymlREADME.mdconfig/config.tomldocs/superpowers/plans/2026-04-09-packetloss-history-cutover.mdinternal/auth/auth.gointernal/auth/oidc.gointernal/broadcaster/broadcaster.gointernal/config/config.gointernal/database/database.gointernal/database/database_test.gointernal/database/migrations/migrations.gointernal/database/user.gointernal/monitor/tailscale_discovery.gointernal/notifications/notifications.gointernal/server/auth.gointernal/server/auth_oidc.gointernal/server/server.gointernal/speedtest/iperf.gointernal/speedtest/progress_broadcaster.gointernal/speedtest/traceroute.gointernal/speedtest/traceroute_test.gointernal/speedtest/types.gointernal/tailscale/tailscale.gointernal/types/types.gointernal/utils/crypto.gointernal/utils/tailscale.gopkg/migrator/migrator.goscripts/check_vnstat_data.shweb/build.goweb/package.jsonweb/src/api/auth.tsweb/src/api/monitor.tsweb/src/api/notifications.tsweb/src/api/tailscale.tsweb/src/components/Footer.tsxweb/src/components/Main.tsxweb/src/components/auth/Login.tsxweb/src/components/monitor/MonitorSystemInfo.tsxweb/src/components/monitor/tabs/MonitorOverviewTab.tsxweb/src/components/settings/notifications/index.tsweb/src/components/speedtest/traceroute/constants/tracerouteConstants.tsweb/src/components/speedtest/traceroute/utils/serverUtils.tsweb/src/components/speedtest/traceroute/utils/tracerouteUtils.tsweb/src/constants/monitorRefreshIntervals.tsweb/src/types/pwa.d.tsweb/src/types/service-worker.d.tsweb/src/types/speedtest.tsweb/src/types/types.tsweb/src/utils/agentIcons.tsxweb/src/utils/countryFlags.tsweb/src/utils/darkMode.tsweb/src/utils/timeSettings.ts
💤 Files with no reviewable changes (28)
- web/src/api/tailscale.ts
- scripts/check_vnstat_data.sh
- web/src/types/types.ts
- internal/broadcaster/broadcaster.go
- docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md
- internal/speedtest/traceroute_test.go
- internal/types/types.go
- web/src/types/pwa.d.ts
- internal/utils/crypto.go
- internal/speedtest/progress_broadcaster.go
- web/src/api/notifications.ts
- web/src/components/speedtest/traceroute/utils/tracerouteUtils.ts
- web/src/api/monitor.ts
- internal/server/server.go
- internal/speedtest/types.go
- web/src/types/service-worker.d.ts
- web/package.json
- web/src/components/speedtest/traceroute/constants/tracerouteConstants.ts
- internal/speedtest/iperf.go
- web/src/api/auth.ts
- web/src/types/speedtest.ts
- web/build.go
- internal/auth/oidc.go
- config/config.toml
- internal/database/database_test.go
- internal/notifications/notifications.go
- internal/speedtest/traceroute.go
- internal/database/database.go
🚧 Files skipped from review as they are similar to previous changes (21)
- internal/monitor/tailscale_discovery.go
- web/src/components/Footer.tsx
- web/src/utils/countryFlags.ts
- .golangci.yml
- web/src/components/speedtest/traceroute/utils/serverUtils.ts
- web/src/constants/monitorRefreshIntervals.ts
- internal/utils/tailscale.go
- web/src/components/monitor/MonitorSystemInfo.tsx
- internal/auth/auth.go
- web/src/components/auth/Login.tsx
- internal/database/user.go
- internal/server/auth.go
- web/src/components/Main.tsx
- web/src/utils/agentIcons.tsx
- web/src/components/monitor/tabs/MonitorOverviewTab.tsx
- web/src/utils/darkMode.ts
- internal/server/auth_oidc.go
- README.md
- internal/database/migrations/migrations.go
- web/src/utils/timeSettings.ts
- pkg/migrator/migrator.go
Comment on lines
63
to
64
| addr := fmt.Sprintf("%s:%d", status.Self.TailscaleIPs[0], port) | ||
| return net.Listen("tcp", addr) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'func ListenOnTailscale|TailscaleIPs\[0\]|JoinHostPort' internal/tailscale/tailscale.go
fd -t f --glob '*tailscale*_test.go' internalRepository: autobrr/netronome
Length of output: 703
🏁 Script executed:
#!/usr/bin/env batch
set -euo pipefail
echo '== package imports and ListenOnTailscale context =='
sed -n '1,80p' internal/tailscale/tailscale.go
echo
echo '== existing tailscale tests =='
for f in internal/config/tailscale_config_test.go internal/utils/tailscale_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
echo
echo '== deterministic Go net.JoinHostPort equivalent behavior probe (Go reference source not run, use installed Go if available) =='
if command -v go >/dev/null 2>&1; then
tmp="$(mktemp -d)"
cat > "$tmp/probe.go" <<'EOF'
package main
import (
"fmt"
"net"
)
func main() {
for _, ip := range []string{"100.64.1.2", "fd7a:115c:a1e0:ab12:4843:cd96:6240:102"} {
port := 9000
invalid := fmt.Sprintf("%s:%d", ip, port)
valid := net.JoinHostPort(ip, "9000")
fmt.Printf("ip=%s invalid=%q valid=%q\n", ip, invalid, valid)
_, err := net.ResolveTCPAddr("tcp", invalid)
fmt.Printf("resolve invalid err: %T %v\n", err, err)
r, err := net.ResolveTCPAddr("tcp", valid)
fmt.Printf("resolve valid addr: %v err: %T %v\n", r, err, err)
if l, err := net.Listen("tcp", valid); err != nil {
fmt.Printf("listen err: %T %v\n", err, err)
} else {
fmt.Printf("listen ok addr=%s resolved=%s\n", l.Addr(), l.Addr().String())
l.Close()
}
}
}
EOF
(cd "$tmp" && go run probe.go)
else
echo "go not installed"
fiRepository: autobrr/netronome
Length of output: 12215
🏁 Script executed:
#!/usr/bin/bash
set -euo pipefail
echo '== package imports and ListenOnTailscale context =='
sed -n '1,80p' internal/tailscale/tailscale.go
echo
echo '== existing tailscale tests =='
for f in internal/config/tailscale_config_test.go internal/utils/tailscale_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
echo
echo '== deterministic net.JoinHostPort behavior probe =='
temp="$(mktemp -d)"
cat > "$temp/probe.go" <<'EOF'
package main
import (
"fmt"
"net"
)
func main() {
for _, ip := range []string{"100.64.1.2", "fd7a:115c:a1e0:ab12:4843:cd96:6240:102"} {
port := 9000
invalid := fmt.Sprintf("%s:%d", ip, port)
valid := net.JoinHostPort(ip, "9000")
fmt.Printf("ip=%s invalid=%q valid=%q\n", ip, invalid, valid)
_, err := net.ResolveTCPAddr("tcp", invalid)
fmt.Printf("resolve invalid err: %T %v\n", err, err)
r, err := net.ResolveTCPAddr("tcp", valid)
fmt.Printf("resolve valid addr: %v err: %T %v\n", r, err, err)
if listener, err := net.Listen("tcp", valid); err != nil {
fmt.Printf("listen err: %T %v\n", err, err)
} else {
fmt.Printf("listen ok addr=%s resolved=%s\n", listener.Addr(), listener.Addr().String())
listener.Close()
}
}
}
EOF
if command -v go >/dev/null 2>&1; then
(cd "$temp" && go run probe.go)
else
echo "go not installed"
fiRepository: autobrr/netronome
Length of output: 12140
Use net.JoinHostPort for the listener address.
fmt.Sprintf("%s:%d", ...) accepts IPv6 Tailscale addresses as-is, which Go parses invalidly because the IPv6 literal needs brackets. This makes ListenOnTailscale fail to parse the address before listening; add IPv4 and IPv6 table-driven regression coverage for this function.
Proposed fix
+ "strconv"
+
- addr := fmt.Sprintf("%s:%d", status.Self.TailscaleIPs[0], port)
+ addr := net.JoinHostPort(status.Self.TailscaleIPs[0].String(), strconv.Itoa(port))
return net.Listen("tcp", addr)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| addr := fmt.Sprintf("%s:%d", status.Self.TailscaleIPs[0], port) | |
| return net.Listen("tcp", addr) | |
| addr := net.JoinHostPort(status.Self.TailscaleIPs[0].String(), strconv.Itoa(port)) | |
| return net.Listen("tcp", addr) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 64-64: net.Listen must not be called. use (*net.ListenConfig).Listen
(noctx)
🤖 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 `@internal/tailscale/tailscale.go` around lines 63 - 64, Update
ListenOnTailscale to build its listener address with net.JoinHostPort instead of
fmt.Sprintf, preserving the selected Tailscale IP and port for both IPv4 and
IPv6. Add table-driven regression tests covering both address families and
verify the function reaches net.Listen with a valid host-port address.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/Main.tsx (1)
59-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore the public theme when leaving public mode.
The public-state effect only flags late promise updates; it does not restore the prior theme when
Mainunmounts from/public. Add effect cleanup or route-level reset so protected routes cannot remain styled with the public theme.🤖 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 `@web/src/components/Main.tsx` around lines 59 - 73, Update the public-mode effect in Main’s useEffect so its cleanup restores the previously active non-public theme when isPublic becomes false or the component unmounts, while retaining the active guard for late getPublicTheme results. Use the existing theme-application/reset mechanism rather than introducing a separate styling path.
🤖 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.
Outside diff comments:
In `@web/src/components/Main.tsx`:
- Around line 59-73: Update the public-mode effect in Main’s useEffect so its
cleanup restores the previously active non-public theme when isPublic becomes
false or the component unmounts, while retaining the active guard for late
getPublicTheme results. Use the existing theme-application/reset mechanism
rather than introducing a separate styling path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51865194-3c7f-4b75-9011-252352138638
📒 Files selected for processing (11)
README.mdconfig/config.tomlinternal/config/config.gointernal/notifications/notifications.gointernal/server/server.goweb/src/api/notifications.tsweb/src/components/Main.tsxweb/src/components/monitor/tabs/MonitorOverviewTab.tsxweb/src/components/speedtest/traceroute/utils/serverUtils.tsweb/src/utils/darkMode.tsweb/src/utils/timeSettings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/speedtest/traceroute/utils/serverUtils.ts
- README.md
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
1 participant
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.
Repo-wide over-engineering audit. No behavior change: 80 insertions, 1393 deletions.
Backend drops the unused
internal/broadcasterpackage and its pass-through wrapper (the real wiring is a plain func value), three dead interfaces, some unused query helpers, and a duplicategetMigrationVersion. The tailscaleClientinterface and its two identical wrapper structs collapse into a type alias forlocal.Client, which both already wrapped. The batch traceroute parsers go too — there is one exec path and it uses the streaming parser, whoseparseHopLinealready handles Windows and carries its own IPv6 test. Seven config knobs that were parsed, defaulted and written to the generated TOML but never read are gone along with their README entries; unknown keys are ignored on decode, so existing config files keep working.tsaddr.IsTailscaleIPreplaces a hand-rolled CIDR check, and stdlibcrypto/rand.TextreplacesGenerateSecureToken, dropping four error branches — the session secret concatenates two calls to keep its previous ~256 bits.Frontend removes eight dependencies:
@mui/materialplus its two emotion peers (used for a singleContainerthat Tailwind classes replace exactly),@headlessui/react(zero imports), and four@fortawesomepackages covering six iconsreact-iconsalready provides. Bundle drops 164 kB, 50 kB gzipped.Verified:
go build,go vet,go test ./...green;tsc --noEmitclean;pnpm buildsucceeds; lint is 25 problems, identical to develop. UI checked in a browser against the running binary — the replaced container measures max-width 1536px, 24px gutters, border-box, centering exactly, matching MUI'sContainer maxWidth="xl". Cross-compiles clean on darwin, linux, windows and freebsd.Draft because two icon swaps (Linux/Apple/OpenID/Laptop) sit behind auth or need configured agents, so they are covered by typecheck and build but not visually confirmed.
Summary by CodeRabbit
New Features
Improvements