🎉 v1.1.0 #13

2025-06-12T17:10:46+08:00

sky22333 commented

2025-06-12 17:10:46 +08:00

(Migrated from github.com)

前端重构
修复了一些BUG
后端大量优化

Summary by CodeRabbit

New Features
- Added a new Docker offline image download page supporting single and batch downloads with platform selection.
- Introduced an image streaming service enabling direct download of container images in Docker tarball format with multi-architecture support.
- Updated homepage and search pages with modern responsive UI, dark mode, and enhanced navigation including theme and mobile menu toggles.
Bug Fixes
- Removed deprecated Docker image batch download tool and related backend service to streamline functionality.
Refactor
- Simplified GitHub URL rewriting in shell scripts with a new full-content processing approach for improved efficiency.
- Unified proxy handling for shell scripts with selective smart processing and direct streaming fallback.
- Improved cache eviction logic to remove expired entries and perform batch cleanup for better memory management.
- Enhanced rate limiter with double-checked locking for thread-safe and efficient access.
- Streamlined configuration caching to reduce redundant deep copies and suppressed related debug logs.
- Refined Docker proxy and registry request handling by removing redundant comments and logs.
Style
- Eliminated Bootstrap and external fonts, adopting custom CSS variables and transitions for consistent theming and accessibility.
Chores
- Removed open-source license file and acknowledgements section from documentation.
- Updated dependency management by removing unused websocket library and adjusting module requirements.
- Removed skopeo installation from Docker image and service installation script.
- Suppressed verbose startup logs for configuration loading, Docker proxy, and rate limiter initialization.

- 前端重构 - 修复了一些BUG - 后端大量优化  ## Summary by CodeRabbit - **New Features** - Added a new Docker offline image download page supporting single and batch downloads with platform selection. - Introduced an image streaming service enabling direct download of container images in Docker tarball format with multi-architecture support. - Updated homepage and search pages with modern responsive UI, dark mode, and enhanced navigation including theme and mobile menu toggles. - **Bug Fixes** - Removed deprecated Docker image batch download tool and related backend service to streamline functionality. - **Refactor** - Simplified GitHub URL rewriting in shell scripts with a new full-content processing approach for improved efficiency. - Unified proxy handling for shell scripts with selective smart processing and direct streaming fallback. - Improved cache eviction logic to remove expired entries and perform batch cleanup for better memory management. - Enhanced rate limiter with double-checked locking for thread-safe and efficient access. - Streamlined configuration caching to reduce redundant deep copies and suppressed related debug logs. - Refined Docker proxy and registry request handling by removing redundant comments and logs. - **Style** - Eliminated Bootstrap and external fonts, adopting custom CSS variables and transitions for consistent theming and accessibility. - **Chores** - Removed open-source license file and acknowledgements section from documentation. - Updated dependency management by removing unused websocket library and adjusting module requirements. - Removed skopeo installation from Docker image and service installation script. - Suppressed verbose startup logs for configuration loading, Docker proxy, and rate limiter initialization.

coderabbitai[bot] commented

2025-06-12 17:10:53 +08:00

(Migrated from github.com)

Walkthrough

This update removes the custom license and acknowledgements from documentation, refactors proxy handling with a new smart processor for shell scripts, simplifies GitHub URL rewriting, modernizes frontend pages with custom responsive dark mode UI and navigation, replaces the Docker image download UI and service with a new image streaming solution, removes skopeo dependency and related installation steps, and improves concurrency, caching, and rate limiting mechanisms.

Changes

File(s)	Change Summary
README.md	Removed the "🙏 致谢" (Acknowledgements) section referencing GHProxy-Frontend; updated feature descriptions for offline image package and memory cleanup.
src/LICENSE	Deleted the bilingual "WJQserver Studio Open Source License" file.
src/config.go	Added caching layer to `GetConfig` to reduce deep copy overhead; removed explicit config load print; suppressed hot reload print message.
src/main.go	Added image streaming initialization and routes; replaced skopeo routes; refactored `proxyWithRedirect` to use new `ProcessSmart` for ".sh" files with fallback; direct streaming proxy for others; unified header copying and redirect handling; added global panic recovery middleware; replaced static file route to serve "images.html"; enhanced startup log with host, port, rate limit details; added `initHealthRoutes` function.
src/proxysh.go	Removed old line-by-line GitHub URL processing; added `ProcessSmart` for full-content processing with gzip detection and regex URL rewriting; simplified URL transformation logic; removed `IsShellFile` and related functions.
src/public/index.html	Fully restyled with custom CSS and dark mode; restructured layout with sticky navbar, mobile menu toggle, and theme toggle; improved GitHub link generation and clipboard support; added toast notifications and enhanced JS event handling.
src/public/search.html	Removed Bootstrap and custom fonts; added custom CSS color system with light/dark themes; introduced sticky navbar with theme and mobile menu toggles; restyled search UI and pagination; added JS for theme toggling, mobile menu, and outside click dismissal.
src/public/skopeo.html	Removed entire Docker offline image batch download tool HTML page and all embedded scripts.
src/public/images.html	Added new Docker offline image download page with light/dark themes, sticky navbar, single and batch download forms, and JavaScript handling downloads and UI interactions.
src/skopeo_service.go	Removed entire Docker image download service including task management, WebSocket progress updates, concurrency control, and file serving.
src/imagetar.go	Added new image streaming and downloading service supporting single/multi-arch images with concurrency, caching, and Gin HTTP handlers for direct and batch downloads and image info retrieval.
Dockerfile	Removed installation of `skopeo` package from final Alpine image stage.
install-service.sh	Removed `skopeo` from dependency checks and installation commands; now only checks and installs `curl` and `jq`.
src/go.mod	Removed direct dependency on `github.com/gorilla/websocket`; moved `golang.org/x/sync` to indirect dependency.
src/token_cache.go	Added `buildManifestCacheKeyWithPlatform` to include platform info in manifest cache keys; removed comments.
src/docker.go	Removed print statement in `initDockerProxy`; cleaned comments in registry routing and auth options.
src/ratelimiter.go	Removed rate limiter init print statement; refactored `GetLimiter` with double-checked locking for thread-safe, efficient limiter retrieval and update; removed comments.
src/search.go	Modified cache eviction in `Cache.Set` to remove expired entries first, then bulk delete 25% oldest entries if needed; reused single timestamp for entry creation; removed comments.
src/access_control.go	Removed all inline comments from Docker image parsing and matching functions; no logic changes.
src/smart_ratelimit.go	Removed all inline comments; no logic changes.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ProxyServer
    participant SmartProcessor
    participant UpstreamServer

    Client->>ProxyServer: HTTP request (any file)
    ProxyServer->>UpstreamServer: Forward request
    UpstreamServer-->>ProxyServer: Response (body, headers)
    ProxyServer->>SmartProcessor: ProcessSmart(body, isCompressed, host) if ".sh" file
    alt Processing succeeds
        SmartProcessor-->>ProxyServer: Processed content
        ProxyServer->>Client: Return transformed body (chunked)
    else Processing fails or non ".sh" file
        ProxyServer->>Client: Return original body (direct proxy)
    end

Poem

🐇✨
In tunnels deep where code does play,
Scripts now smartly find their way.
Heartbeats ping, connections strong,
Dark mode glows the whole day long.
Links transformed with nimble grace,
Old licenses leave no trace.
Rabbits cheer this lively space! 🥕🎉

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8ffceb7f2b and 118296ee31.

📒 Files selected for processing (1)

README.md (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

README.md

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

## Walkthrough This update removes the custom license and acknowledgements from documentation, refactors proxy handling with a new smart processor for shell scripts, simplifies GitHub URL rewriting, modernizes frontend pages with custom responsive dark mode UI and navigation, replaces the Docker image download UI and service with a new image streaming solution, removes skopeo dependency and related installation steps, and improves concurrency, caching, and rate limiting mechanisms. ## Changes | File(s) | Change Summary | |-------------------------------------------|----------------| | README.md | Removed the "🙏 致谢" (Acknowledgements) section referencing GHProxy-Frontend; updated feature descriptions for offline image package and memory cleanup. | | src/LICENSE | Deleted the bilingual "WJQserver Studio Open Source License" file. | | src/config.go | Added caching layer to `GetConfig` to reduce deep copy overhead; removed explicit config load print; suppressed hot reload print message. | | src/main.go | Added image streaming initialization and routes; replaced skopeo routes; refactored `proxyWithRedirect` to use new `ProcessSmart` for ".sh" files with fallback; direct streaming proxy for others; unified header copying and redirect handling; added global panic recovery middleware; replaced static file route to serve "images.html"; enhanced startup log with host, port, rate limit details; added `initHealthRoutes` function. | | src/proxysh.go | Removed old line-by-line GitHub URL processing; added `ProcessSmart` for full-content processing with gzip detection and regex URL rewriting; simplified URL transformation logic; removed `IsShellFile` and related functions. | | src/public/index.html | Fully restyled with custom CSS and dark mode; restructured layout with sticky navbar, mobile menu toggle, and theme toggle; improved GitHub link generation and clipboard support; added toast notifications and enhanced JS event handling. | | src/public/search.html | Removed Bootstrap and custom fonts; added custom CSS color system with light/dark themes; introduced sticky navbar with theme and mobile menu toggles; restyled search UI and pagination; added JS for theme toggling, mobile menu, and outside click dismissal. | | src/public/skopeo.html | Removed entire Docker offline image batch download tool HTML page and all embedded scripts. | | src/public/images.html | Added new Docker offline image download page with light/dark themes, sticky navbar, single and batch download forms, and JavaScript handling downloads and UI interactions. | | src/skopeo_service.go | Removed entire Docker image download service including task management, WebSocket progress updates, concurrency control, and file serving. | | src/imagetar.go | Added new image streaming and downloading service supporting single/multi-arch images with concurrency, caching, and Gin HTTP handlers for direct and batch downloads and image info retrieval. | | Dockerfile | Removed installation of `skopeo` package from final Alpine image stage. | | install-service.sh | Removed `skopeo` from dependency checks and installation commands; now only checks and installs `curl` and `jq`. | | src/go.mod | Removed direct dependency on `github.com/gorilla/websocket`; moved `golang.org/x/sync` to indirect dependency. | | src/token_cache.go | Added `buildManifestCacheKeyWithPlatform` to include platform info in manifest cache keys; removed comments. | | src/docker.go | Removed print statement in `initDockerProxy`; cleaned comments in registry routing and auth options. | | src/ratelimiter.go | Removed rate limiter init print statement; refactored `GetLimiter` with double-checked locking for thread-safe, efficient limiter retrieval and update; removed comments. | | src/search.go | Modified cache eviction in `Cache.Set` to remove expired entries first, then bulk delete 25% oldest entries if needed; reused single timestamp for entry creation; removed comments. | | src/access_control.go | Removed all inline comments from Docker image parsing and matching functions; no logic changes. | | src/smart_ratelimit.go | Removed all inline comments; no logic changes. | ## Sequence Diagram(s) ```mermaid sequenceDiagram participant Client participant ProxyServer participant SmartProcessor participant UpstreamServer Client->>ProxyServer: HTTP request (any file) ProxyServer->>UpstreamServer: Forward request UpstreamServer-->>ProxyServer: Response (body, headers) ProxyServer->>SmartProcessor: ProcessSmart(body, isCompressed, host) if ".sh" file alt Processing succeeds SmartProcessor-->>ProxyServer: Processed content ProxyServer->>Client: Return transformed body (chunked) else Processing fails or non ".sh" file ProxyServer->>Client: Return original body (direct proxy) end ``` ## Poem > 🐇✨ > In tunnels deep where code does play, > Scripts now smartly find their way. > Heartbeats ping, connections strong, > Dark mode glows the whole day long. > Links transformed with nimble grace, > Old licenses leave no trace. > Rabbits cheer this lively space! 🥕🎉  --- <details> <summary>📜 Recent review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8ffceb7f2b6f4f9ea47833d64aa952018f70f4d9 and 118296ee3126a5b1f1520388c4625375251def20. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `README.md` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * README.md </details> </details>     --- 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. <details> <summary>❤️ Share</summary> - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code) </details> <details> <summary>🪧 Tips</summary> ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai?utm_source=oss&utm_medium=github&utm_campaign=sky22333/hubproxy&utm_content=13): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit <commit_id>, please review it.` - `Explain this complex logic.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai explain this code block.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.` - `@coderabbitai read src/utils.ts and explain its main purpose.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` ### Support Need help? Create a ticket on our [support page](https://www.coderabbit.ai/contact-us/support) for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (Invoked using PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai generate docstrings` to [generate docstrings](https://docs.coderabbit.ai/finishing-touches/docstrings) for this PR. - `@coderabbitai generate sequence diagram` to generate a sequence diagram of the changes in this PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. ### Other keywords and placeholders - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. - Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description. - Add `@coderabbitai` anywhere in the PR title to generate the title automatically. ### CodeRabbit Configuration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://docs.coderabbit.ai) for detailed information on how to use CodeRabbit. - Join our [Discord Community](http://discord.gg/coderabbit) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements. </details>

coderabbitai[bot] commented

2025-06-12 17:19:18 +08:00

(Migrated from github.com)

Note

Generated docstrings for this pull request at https://github.com/sky22333/hubproxy/pull/14

> [!NOTE] > Generated docstrings for this pull request at https://github.com/sky22333/hubproxy/pull/14

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-12 18:04:41 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 3

🔭 Outside diff range comments (1)

src/skopeo_service.go (1)
1406-1444: ⚠️ Potential issue

Deadlock when cleaning up connections

cleanupWebSocketConnections holds clientLock for the whole sweep and calls client.close(), but close() itself tries to clientLock.Lock() again.
Result: the goroutine stops forever and no further connections are cleaned.
- clientLock.Lock()
- ...
- for _, taskID := range disconnectedClients {
-     if client, exists := clients[taskID]; exists {
-         client.close()  // ← re-enters clientLock
-     }
- }
- clientLock.Unlock()
+ clientLock.Lock()
+ ...
+ stale := append([]string(nil), disconnectedClients...)
+ clientLock.Unlock()
+
+for _, taskID := range stale {
+    if client, exists := clients[taskID]; exists {
+        client.close()
+    }
+}

🧹 Nitpick comments (4)

src/skopeo_service.go (1)

172-188: Unchecked SetReadDeadline errors

Both invocations of conn.SetReadDeadline ignore the returned error (lines 172, 187).
If the underlying net.Conn is gone this returns an error that should trigger client.close() immediately.

🧰 Tools

🪛 golangci-lint (1.64.8)

172-172: Error return value of conn.SetReadDeadline is not checked

(errcheck)

187-187: Error return value of conn.SetReadDeadline is not checked

(errcheck)

src/public/skopeo.html (1)

676-683: Potential selector collision

const navLinks = document.getElementById('navLinks'); is declared twice (once earlier for DOMContentLoaded, again in the theme / mobile menu block).
While harmless in most browsers, shadowing the variable can confuse minifiers or future edits. Consider re-using the first binding.

src/public/search.html (1)

665-704: Duplicate variable declarations

Same issue as in skopeo page – navLinks & mobileMenuToggle are re-declared; prefer const navLinks … once to avoid shadowing.
src/proxysh.go (1)
217-219: Avoid storing large slices by value in sync.Pool

bufferPool.Put(buffer) stores a copy of the 32 KB slice header on every
call, triggering SA6002. Store a pointer instead:
-buffer := sp.bufferPool.Get().([]byte)
-…
-sp.bufferPool.Put(buffer)
+bufPtr := sp.bufferPool.Get().(*[]byte)
+buffer := *bufPtr
+…
+sp.bufferPool.Put(bufPtr)
This keeps allocations near-zero and silences the linter.

🧰 Tools

🪛 golangci-lint (1.64.8)

218-218: SA6002: argument should be pointer-like to avoid allocations

(staticcheck)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db9d11a44e and a8498ad24a.

📒 Files selected for processing (9)

README.md (0 hunks)
src/LICENSE (0 hunks)
src/config.go (1 hunks)
src/main.go (2 hunks)
src/proxysh.go (1 hunks)
src/public/index.html (2 hunks)
src/public/search.html (24 hunks)
src/public/skopeo.html (18 hunks)
src/skopeo_service.go (6 hunks)

💤 Files with no reviewable changes (2)

README.md
src/LICENSE

🧰 Additional context used

🪛 golangci-lint (1.64.8)

src/skopeo_service.go

172-172: Error return value of conn.SetReadDeadline is not checked

(errcheck)

187-187: Error return value of conn.SetReadDeadline is not checked

(errcheck)

200-200: S1000: should use for range instead of for { select {} }

(gosimple)

src/proxysh.go

218-218: SA6002: argument should be pointer-like to avoid allocations

(staticcheck)

🔇 Additional comments (3)

src/main.go (1)

238-244: Header mutation: removing Content-Encoding without re-encoding

When processedSize>0 you unconditionally strip Content-Encoding.
If the original body was not actually decoded inside ProcessSmart, clients will now receive gzipped bytes but think it’s plain text.
Double-check that ProcessSmart always returns decoded content before clearing the header.

src/config.go (1)

220-222: Minor message tweak looks good

Only a user-visible wording change – no functional impact. 👍
src/public/index.html (1)

753-770: Verify URL-rewriting logic – double https:// may break downloads

formatGithubLink() builds
https://${currentHost}/https://github.com/... for inputs that already contain https://.
Browsers treat the part after the first scheme as path text, not as another URL, e.g.
https://acc.host/https://github.com/user/repo
curl/wget inside the generated shell scripts will hit your service at
/https://github.com/..., which is probably what you want, but the extra
protocol fragment is still embedded in the path and will later be duplicated
again by transformURL() on the server (see proxysh.go), yielding
acc.host/https://acc.host/https://github.com/...
Please double-check both client and server rewrite rules stay consistent and
produce a single prefixed URL of the form
https://acc.host/github.com/user/repo…
or whatever your proxy expects.

**Actionable comments posted: 3** <details> <summary>🔭 Outside diff range comments (1)</summary><blockquote> <details> <summary>src/skopeo_service.go (1)</summary><blockquote> `1406-1444`: _⚠️ Potential issue_ **Deadlock when cleaning up connections** `cleanupWebSocketConnections` holds `clientLock` for the whole sweep _and_ calls `client.close()`, but `close()` itself tries to `clientLock.Lock()` again. Result: the goroutine stops forever and no further connections are cleaned. ```diff - clientLock.Lock() - ... - for _, taskID := range disconnectedClients { - if client, exists := clients[taskID]; exists { - client.close() // ← re-enters clientLock - } - } - clientLock.Unlock() + clientLock.Lock() + ... + stale := append([]string(nil), disconnectedClients...) + clientLock.Unlock() + +for _, taskID := range stale { + if client, exists := clients[taskID]; exists { + client.close() + } +} ``` </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/skopeo_service.go (1)</summary><blockquote> `172-188`: **Unchecked SetReadDeadline errors** Both invocations of `conn.SetReadDeadline` ignore the returned error (lines 172, 187). If the underlying net.Conn is gone this returns an error that should trigger `client.close()` immediately. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> 172-172: Error return value of `conn.SetReadDeadline` is not checked (errcheck) --- 187-187: Error return value of `conn.SetReadDeadline` is not checked (errcheck) </details> </details> </blockquote></details> <details> <summary>src/public/skopeo.html (1)</summary><blockquote> `676-683`: **Potential selector collision** `const navLinks = document.getElementById('navLinks');` is declared twice (once earlier for `DOMContentLoaded`, again in the theme / mobile menu block). While harmless in most browsers, shadowing the variable can confuse minifiers or future edits. Consider re-using the first binding. </blockquote></details> <details> <summary>src/public/search.html (1)</summary><blockquote> `665-704`: **Duplicate variable declarations** Same issue as in skopeo page – `navLinks` & `mobileMenuToggle` are re-declared; prefer `const navLinks …` once to avoid shadowing. </blockquote></details> <details> <summary>src/proxysh.go (1)</summary><blockquote> `217-219`: **Avoid storing large slices by value in `sync.Pool`** `bufferPool.Put(buffer)` stores a *copy* of the 32 KB slice header on every call, triggering SA6002. Store a pointer instead: ```diff -buffer := sp.bufferPool.Get().([]byte) -… -sp.bufferPool.Put(buffer) +bufPtr := sp.bufferPool.Get().(*[]byte) +buffer := *bufPtr +… +sp.bufferPool.Put(bufPtr) ``` This keeps allocations near-zero and silences the linter. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> 218-218: SA6002: argument should be pointer-like to avoid allocations (staticcheck) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between db9d11a44ee40136e0754cd6bb6db27ca9e05dc2 and a8498ad24ab30c7873e0ccfdbf1475eb9b962e58. </details> <details> <summary>📒 Files selected for processing (9)</summary> * `README.md` (0 hunks) * `src/LICENSE` (0 hunks) * `src/config.go` (1 hunks) * `src/main.go` (2 hunks) * `src/proxysh.go` (1 hunks) * `src/public/index.html` (2 hunks) * `src/public/search.html` (24 hunks) * `src/public/skopeo.html` (18 hunks) * `src/skopeo_service.go` (6 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * README.md * src/LICENSE </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> <details> <summary>src/skopeo_service.go</summary> 172-172: Error return value of `conn.SetReadDeadline` is not checked (errcheck) --- 187-187: Error return value of `conn.SetReadDeadline` is not checked (errcheck) --- 200-200: S1000: should use for range instead of for { select {} } (gosimple) </details> <details> <summary>src/proxysh.go</summary> 218-218: SA6002: argument should be pointer-like to avoid allocations (staticcheck) </details> </details> </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>src/main.go (1)</summary> `238-244`: **Header mutation: removing Content-Encoding without re-encoding** When `processedSize>0` you unconditionally strip `Content-Encoding`. If the original body was _not_ actually decoded inside `ProcessSmart`, clients will now receive gzipped bytes but think it’s plain text. Double-check that `ProcessSmart` always returns _decoded_ content before clearing the header. </details> <details> <summary>src/config.go (1)</summary> `220-222`: **Minor message tweak looks good** Only a user-visible wording change – no functional impact. 👍 </details> <details> <summary>src/public/index.html (1)</summary> `753-770`: **Verify URL-rewriting logic – double `https://` may break downloads** `formatGithubLink()` builds `https://${currentHost}/https://github.com/...` for inputs that already contain `https://`. Browsers treat the part after the first scheme as *path* text, not as another URL, e.g. ``` https://acc.host/https://github.com/user/repo ``` `curl`/`wget` inside the generated shell scripts will hit your service at `/https://github.com/...`, which is probably what you want, **but** the extra protocol fragment is still embedded in the path and will later be duplicated again by `transformURL()` on the server (see proxysh.go), yielding ``` acc.host/https://acc.host/https://github.com/... ``` Please double-check both client and server rewrite rules stay consistent and produce a single prefixed URL of the form ``` https://acc.host/github.com/user/repo… ``` or whatever your proxy expects. </details> </blockquote></details> </details>

src/main.go

coderabbitai[bot] (Migrated from github.com) commented

2025-06-12 18:04:40 +08:00

⚠️ Potential issue

Fallback after ProcessSmart may return a truncated file

ProcessSmart consumes resp.Body.
If it returns an error you immediately enter the “direct proxy” branch and io.Copy the same (now-drained) resp.Body; the client receives an empty / partial response.

Fix ideas:

- processedBody, processedSize, err := ProcessSmart(resp.Body, ...)
+ var buf bytes.Buffer
+ tee := io.TeeReader(resp.Body, &buf)
+ processedBody, processedSize, err := ProcessSmart(tee, ...)

or re-issue the upstream request in the fallback path.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In src/main.go around lines 216 to 235, the fallback logic after ProcessSmart
fails attempts to copy from the already consumed resp.Body, resulting in an
empty or partial response to the client. To fix this, modify the fallback path
to re-issue the upstream request to get a fresh resp.Body before copying it to
the client, ensuring the full original response is forwarded correctly.

_⚠️ Potential issue_ **Fallback after ProcessSmart may return a truncated file** `ProcessSmart` consumes `resp.Body`. If it returns an error you immediately enter the “direct proxy” branch and `io.Copy` the _same_ (now-drained) `resp.Body`; the client receives an empty / partial response. Fix ideas: ```diff - processedBody, processedSize, err := ProcessSmart(resp.Body, ...) + var buf bytes.Buffer + tee := io.TeeReader(resp.Body, &buf) + processedBody, processedSize, err := ProcessSmart(tee, ...) ``` or re-issue the upstream request in the fallback path. > Committable suggestion skipped: line range outside the PR's diff. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/main.go around lines 216 to 235, the fallback logic after ProcessSmart fails attempts to copy from the already consumed resp.Body, resulting in an empty or partial response to the client. To fix this, modify the fallback path to re-issue the upstream request to get a fresh resp.Body before copying it to the client, ensuring the full original response is forwarded correctly. ``` </details>

src/proxysh.go

						
				@@ -1,7 +1,7 @@

				package main

coderabbitai[bot] (Migrated from github.com) commented

2025-06-12 18:04:40 +08:00

⚠️ Potential issue

transformURL drops the scheme ⇒ generated links become invalid

return cleanHost + "/" + url produces a host-relative URL:

acc.host/https://github.com/...

Shell tools (curl, wget) require an explicit scheme; without it they treat
the value as a file path. Fix:

-// 构建新URL
-return cleanHost + "/" + url
+// 构建新URL
+return "https://" + cleanHost + "/" + url

Also consider stripping any existing https:// prefix from url once, so
you never end up with duplicated protocols when the client already supplied
https://….

📝 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.

func (sp *SmartProcessor) transformURL(url, host string) string {
    // 避免重复处理
    if strings.Contains(url, host) {
        return url
    }

    // 协议标准化
    if !strings.HasPrefix(url, "https://") {
        if strings.HasPrefix(url, "http://") {
            url = "https://" + url[7:]
        } else if !strings.HasPrefix(url, "//") {
            url = "https://" + url
        }
    }

    // Host清理
    cleanHost := strings.TrimPrefix(host, "https://")
    cleanHost = strings.TrimPrefix(cleanHost, "http://")
    cleanHost = strings.TrimSuffix(cleanHost, "/")

    // 构建新URL
    return "https://" + cleanHost + "/" + url
}

🤖 Prompt for AI Agents

In src/proxysh.go around lines 306 to 328, the transformURL function incorrectly
concatenates the cleaned host and the full URL including its scheme, resulting
in invalid URLs like "acc.host/https://github.com/...". To fix this, ensure the
returned URL includes the scheme explicitly at the start, for example by
prefixing with "https://". Additionally, strip any existing "https://" or
"http://" prefix from the input URL before concatenation to avoid duplicated
protocols in the final URL.

_⚠️ Potential issue_ **`transformURL` drops the scheme ⇒ generated links become invalid** `return cleanHost + "/" + url` produces a host-relative URL: ``` acc.host/https://github.com/... ``` Shell tools (`curl`, `wget`) require an explicit scheme; without it they treat the value as a file path. Fix: ```diff -// 构建新URL -return cleanHost + "/" + url +// 构建新URL +return "https://" + cleanHost + "/" + url ``` Also consider stripping *any* existing `https://` prefix from `url` once, so you never end up with duplicated protocols when the client already supplied `https://…`.  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion func (sp *SmartProcessor) transformURL(url, host string) string { // 避免重复处理 if strings.Contains(url, host) { return url } // 协议标准化 if !strings.HasPrefix(url, "https://") { if strings.HasPrefix(url, "http://") { url = "https://" + url[7:] } else if !strings.HasPrefix(url, "//") { url = "https://" + url } } // Host清理 cleanHost := strings.TrimPrefix(host, "https://") cleanHost = strings.TrimPrefix(cleanHost, "http://") cleanHost = strings.TrimSuffix(cleanHost, "/") // 构建新URL return "https://" + cleanHost + "/" + url } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/proxysh.go around lines 306 to 328, the transformURL function incorrectly concatenates the cleaned host and the full URL including its scheme, resulting in invalid URLs like "acc.host/https://github.com/...". To fix this, ensure the returned URL includes the scheme explicitly at the start, for example by prefixing with "https://". Additionally, strip any existing "https://" or "http://" prefix from the input URL before concatenation to avoid duplicated protocols in the final URL. ``` </details>

src/skopeo_service.go

coderabbitai[bot] (Migrated from github.com) commented

2025-06-12 18:04:40 +08:00

🛠️ Refactor suggestion

Concurrent writes to the same WebSocket

startHeartbeat sends Ping frames with c.Conn.WriteMessage, while writePump writes normal messages.
Gorilla WS requires exactly one writer – concurrent writes will panic with “concurrent write to websocket”.

Introduce a dedicated writeMu in Client and wrap every WriteMessage / WriteControl call, or move the ping logic into writePump.

🧰 Tools

🪛 golangci-lint (1.64.8)

200-200: S1000: should use for range instead of for { select {} }

(gosimple)

🤖 Prompt for AI Agents

In src/skopeo_service.go around lines 196 to 231, the startHeartbeat function
writes Ping messages to the WebSocket concurrently with other writes from
writePump, causing potential panics due to concurrent writes. To fix this, add a
dedicated mutex (e.g., writeMu) to the Client struct and wrap all calls to
WriteMessage and WriteControl with this mutex to serialize writes, or
alternatively, move the ping sending logic into the writePump goroutine to
ensure only one writer handles all WebSocket writes.

_🛠️ Refactor suggestion_ **Concurrent writes to the same WebSocket** `startHeartbeat` sends `Ping` frames with `c.Conn.WriteMessage`, while `writePump` writes normal messages. Gorilla WS requires _exactly one writer_ – concurrent writes will panic with “concurrent write to websocket”. Introduce a dedicated `writeMu` in `Client` and wrap **every** `WriteMessage` / `WriteControl` call, or move the ping logic into `writePump`. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> 200-200: S1000: should use for range instead of for { select {} } (gosimple) </details> </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/skopeo_service.go around lines 196 to 231, the startHeartbeat function writes Ping messages to the WebSocket concurrently with other writes from writePump, causing potential panics due to concurrent writes. To fix this, add a dedicated mutex (e.g., writeMu) to the Client struct and wrap all calls to WriteMessage and WriteControl with this mutex to serialize writes, or alternatively, move the ping sending logic into the writePump goroutine to ensure only one writer handles all WebSocket writes. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 08:27:52 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 5

🧹 Nitpick comments (4)

src/imagetar.go (1)

22-26: Unused concurrency field

ImageStreamer.concurrency is set but never referenced – remove or implement a worker
pool for simultaneous layer fetches.
src/public/images.html (2)
638-643: showStatus drops existing classes

element.className = … nukes all previous classes (e.g., responsiveness helpers).
Use classList to toggle only the status classes.
- element.className = `status status-${type}`;
+ element.classList.remove('status-success','status-error','status-warning');
+ element.classList.add('status', `status-${type}`);
667-676: Safer URL encoding for image names

Only slashes are replaced with underscores. : and @ remain and may confuse reverse
proxies or browsers (: can be parsed as port spec). Consider full segment encoding:
const encodedImage = encodeURIComponent(imageName);
let url = `/api/image/download/${encodedImage}`;
Corresponding server route should url.PathUnescape.
src/public/index.html (1)

783-799: Deprecated fallback path

document.execCommand('copy') is deprecated; modern browsers without Clipboard API are
rare. Consider dropping the fallback or feature-detect and warn users instead.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0dbf304e2 and 80b3f8959f.

⛔ Files ignored due to path filters (1)

src/go.sum is excluded by !**/*.sum

📒 Files selected for processing (10)

Dockerfile (0 hunks)
install-service.sh (2 hunks)
src/go.mod (1 hunks)
src/imagetar.go (1 hunks)
src/main.go (3 hunks)
src/public/images.html (1 hunks)
src/public/index.html (2 hunks)
src/public/search.html (24 hunks)
src/public/skopeo.html (0 hunks)
src/skopeo_service.go (0 hunks)

💤 Files with no reviewable changes (3)

Dockerfile
src/public/skopeo.html
src/skopeo_service.go

✅ Files skipped from review due to trivial changes (1)

src/go.mod

🚧 Files skipped from review as they are similar to previous changes (2)

src/main.go
src/public/search.html

🔇 Additional comments (3)

install-service.sh (2)

65-69: Drop skopeo from dependency checks
The loop now only verifies curl, jq, and tar, which aligns with the removal of skopeo-related functionality.

🧰 Tools

🪛 Shellcheck (0.10.0)

[warning] 67-67: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.

(SC2206)

82-87: Adjust post-install dependency re-check
The re-check loop correctly omits skopeo and validates only the remaining tools (curl, jq, tar).
src/public/index.html (1)

759-773: http:// links are forced to https://

formatGithubLink prepends https://{host}/ regardless of the original scheme, turning
http://… inputs into https://{host}/http://… – double scheme but still “http://” inside.
If backend expects plain http:// you may keep the original scheme:
formattedLink = `${window.location.protocol}//${currentHost}/${link}`;

**Actionable comments posted: 5** <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/imagetar.go (1)</summary><blockquote> `22-26`: **Unused `concurrency` field** `ImageStreamer.concurrency` is set but never referenced – remove or implement a worker pool for simultaneous layer fetches. </blockquote></details> <details> <summary>src/public/images.html (2)</summary><blockquote> `638-643`: **showStatus drops existing classes** `element.className = …` nukes all previous classes (e.g., responsiveness helpers). Use `classList` to toggle only the status classes. ```diff - element.className = `status status-${type}`; + element.classList.remove('status-success','status-error','status-warning'); + element.classList.add('status', `status-${type}`); ``` --- `667-676`: **Safer URL encoding for image names** Only slashes are replaced with underscores. `:` and `@` remain and may confuse reverse proxies or browsers (`:` can be parsed as port spec). Consider full segment encoding: ```js const encodedImage = encodeURIComponent(imageName); let url = `/api/image/download/${encodedImage}`; ``` Corresponding server route should `url.PathUnescape`. </blockquote></details> <details> <summary>src/public/index.html (1)</summary><blockquote> `783-799`: **Deprecated fallback path** `document.execCommand('copy')` is deprecated; modern browsers without Clipboard API are rare. Consider dropping the fallback or feature-detect and warn users instead. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between e0dbf304e2b9097323aac5228db62dc7bc86facc and 80b3f8959ffbb169e6d35e29cdee2236b0521a24. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `src/go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (10)</summary> * `Dockerfile` (0 hunks) * `install-service.sh` (2 hunks) * `src/go.mod` (1 hunks) * `src/imagetar.go` (1 hunks) * `src/main.go` (3 hunks) * `src/public/images.html` (1 hunks) * `src/public/index.html` (2 hunks) * `src/public/search.html` (24 hunks) * `src/public/skopeo.html` (0 hunks) * `src/skopeo_service.go` (0 hunks) </details> <details> <summary>💤 Files with no reviewable changes (3)</summary> * Dockerfile * src/public/skopeo.html * src/skopeo_service.go </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * src/go.mod </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (2)</summary> * src/main.go * src/public/search.html </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>install-service.sh (2)</summary> `65-69`: **Drop skopeo from dependency checks** The loop now only verifies `curl`, `jq`, and `tar`, which aligns with the removal of skopeo-related functionality. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Shellcheck (0.10.0)</summary> [warning] 67-67: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a. (SC2206) </details> </details> --- `82-87`: **Adjust post-install dependency re-check** The re-check loop correctly omits skopeo and validates only the remaining tools (`curl`, `jq`, `tar`). </details> <details> <summary>src/public/index.html (1)</summary> `759-773`: **`http://` links are forced to `https://`** `formatGithubLink` prepends `https://{host}/` regardless of the original scheme, turning `http://…` inputs into `https://{host}/http://…` – double scheme but still “http://” inside. If backend expects plain `http://` you may keep the original scheme: ```js formattedLink = `${window.location.protocol}//${currentHost}/${link}`; ``` </details> </blockquote></details> </details>

install-service.sh

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 08:27:51 +08:00

⚠️ Potential issue

Include tar in the installation command
The script checks for tar but does not install it; if tar is missing, the subsequent validation will always fail. Add tar to the apt install list.

- apt update && apt install -y curl jq
+ apt update && apt install -y curl jq tar

📝 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.

        apt update && apt install -y curl jq tar
        if [ $? -ne 0 ]; then
            echo -e "${RED}依赖安装失败${NC}"
            exit 1
        fi

🤖 Prompt for AI Agents

In install-service.sh around lines 75 to 79, the script installs curl and jq but
does not include tar, which is required later. Modify the apt install command to
include tar along with curl and jq to ensure all dependencies are installed
properly and the validation does not fail.

_⚠️ Potential issue_ **Include `tar` in the installation command** The script checks for `tar` but does not install it; if `tar` is missing, the subsequent validation will always fail. Add `tar` to the apt install list. ```diff - apt update && apt install -y curl jq + apt update && apt install -y curl jq tar ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion apt update && apt install -y curl jq tar if [ $? -ne 0 ]; then echo -e "${RED}依赖安装失败${NC}" exit 1 fi ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In install-service.sh around lines 75 to 79, the script installs curl and jq but does not include tar, which is required later. Modify the apt install command to include tar along with curl and jq to ensure all dependencies are installed properly and the validation does not fail. ``` </details>

src/imagetar.go

						
				@@ -0,0 +1,932 @@

				package main

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 08:27:52 +08:00

⚠️ Potential issue

Batch tar layout is broken

StreamMultipleImages creates a directory header (image_0_xxx/) but then calls
StreamImageToWriter, which writes files at the root of the tar. Subsequent images will
produce duplicate file names and overwrite each other.

Provide the directory prefix to the inner writer (e.g. via tar.NewWriter(prefixWriter))
or change streamDockerFormat to accept a path prefix.

🤖 Prompt for AI Agents

In src/imagetar.go around lines 553 to 574, the batch tar layout is broken
because the directory header is created for each image, but the image files are
written at the root of the tar, causing filename collisions. To fix this, modify
the code to pass the directory prefix (dirName) to the StreamImageToWriter
function or adjust streamDockerFormat to accept a path prefix, ensuring all
files for each image are written under their respective directory in the tar
archive.

_⚠️ Potential issue_ **Batch tar layout is broken** `StreamMultipleImages` creates a directory header (`image_0_xxx/`) but then calls `StreamImageToWriter`, which writes files at the root of the tar. Subsequent images will produce duplicate file names and overwrite each other. Provide the directory prefix to the inner writer (e.g. via `tar.NewWriter(prefixWriter)`) or change `streamDockerFormat` to accept a path prefix. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 553 to 574, the batch tar layout is broken because the directory header is created for each image, but the image files are written at the root of the tar, causing filename collisions. To fix this, modify the code to pass the directory prefix (dirName) to the StreamImageToWriter function or adjust streamDockerFormat to accept a path prefix, ensuring all files for each image are written under their respective directory in the tar archive. ``` </details>

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 08:27:52 +08:00

🛠️ Refactor suggestion

Resource leak: defer inside tight loop

defer layerReader.Close() is executed after the whole function returns, keeping every
uncompressed layer file open until the end. Large images → many FDs, high memory.

Replace with immediate close:

layerReader, err := layer.Uncompressed()
...
if _, err := io.Copy(tarWriter, layerReader); err != nil { ... }
layerReader.Close()

🤖 Prompt for AI Agents

In src/imagetar.go around lines 274 to 304, the defer statement for closing
layerReader inside the loop causes all layer files to remain open until the
function returns, leading to resource leaks. Replace the defer with an immediate
close by calling layerReader.Close() right after finishing the io.Copy operation
for that layer, ensuring each layerReader is closed before the next iteration.

✅ Addressed in commit 03b3c56

_🛠️ Refactor suggestion_ **Resource leak: `defer` inside tight loop** `defer layerReader.Close()` is executed after the whole function returns, keeping every uncompressed layer file open until the end. Large images → many FDs, high memory. Replace with immediate close: ```go layerReader, err := layer.Uncompressed() ... if _, err := io.Copy(tarWriter, layerReader); err != nil { ... } layerReader.Close() ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 274 to 304, the defer statement for closing layerReader inside the loop causes all layer files to remain open until the function returns, leading to resource leaks. Replace the defer with an immediate close by calling layerReader.Close() right after finishing the io.Copy operation for that layer, ensuring each layerReader is closed before the next iteration. ``` </details>  ✅ Addressed in commit 03b3c56

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 08:27:52 +08:00

⚠️ Potential issue

Cache shortcut creates invalid remote.Descriptor

When a manifest is pulled from cache you manually craft a remote.Descriptor with only Manifest/MediaType.
Methods later invoked (desc.Image(), desc.ImageIndex()) rely on internal lazy fetchers that are now
nil and will panic.

Fix: cache the raw manifest separately but always call remote.Get to obtain a full
descriptor, or re-inject the cached bytes through remote.WithContext + remote.Get using
remote.WithTransport, not by new-struct construction.
At minimum populate the remaining fields (Digest, Size, Descriptor, Client, etc.).

🤖 Prompt for AI Agents

In src/imagetar.go lines 95 to 115, the cached manifest is used to manually
create a remote.Descriptor with only Manifest and MediaType fields set, causing
panics later when methods like desc.Image() or desc.ImageIndex() are called due
to missing internal lazy fetchers. To fix this, avoid constructing the
descriptor struct manually from cache data; instead, always call remote.Get to
obtain a complete descriptor or re-inject the cached manifest bytes by using
remote.WithContext and remote.WithTransport options with remote.Get. This
ensures all necessary fields and internal fetchers are properly initialized.

✅ Addressed in commit e305dd2

_⚠️ Potential issue_ **Cache shortcut creates invalid `remote.Descriptor`** When a manifest is pulled from cache you manually craft a `remote.Descriptor` with only `Manifest`/`MediaType`. Methods later invoked (`desc.Image()`, `desc.ImageIndex()`) rely on internal lazy fetchers that are now nil and will panic. Fix: cache the *raw manifest* separately but always call `remote.Get` to obtain a full descriptor, or re-inject the cached bytes through `remote.WithContext` + `remote.Get` using `remote.WithTransport`, not by new-struct construction. At minimum populate the remaining fields (`Digest`, `Size`, `Descriptor`, `Client`, etc.). <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go lines 95 to 115, the cached manifest is used to manually create a remote.Descriptor with only Manifest and MediaType fields set, causing panics later when methods like desc.Image() or desc.ImageIndex() are called due to missing internal lazy fetchers. To fix this, avoid constructing the descriptor struct manually from cache data; instead, always call remote.Get to obtain a complete descriptor or re-inject the cached manifest bytes by using remote.WithContext and remote.WithTransport options with remote.Get. This ensures all necessary fields and internal fetchers are properly initialized. ``` </details>  ✅ Addressed in commit e305dd2

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 08:27:52 +08:00

⚠️ Potential issue

Compile-time failure: wrong DefaultQuery usage

c.DefaultQuery("tag") requires a default value parameter (key, defaultValue string).
This fails to compile.

-	tag := c.DefaultQuery("tag")
+	tag := c.DefaultQuery("tag", "latest")

📝 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.

	tag := c.DefaultQuery("tag", "latest")

	if tag != "" && !strings.Contains(imageRef, ":") && !strings.Contains(imageRef, "@") {
		imageRef = imageRef + ":" + tag
	} else if !strings.Contains(imageRef, ":") && !strings.Contains(imageRef, "@") {

🤖 Prompt for AI Agents

In src/imagetar.go around lines 397 to 401, the call to c.DefaultQuery("tag") is
incorrect because DefaultQuery requires two parameters: the key and a default
value. Fix this by providing a default value string as the second argument, for
example c.DefaultQuery("tag", ""), to ensure it compiles correctly.

✅ Addressed in commit 03b3c56

_⚠️ Potential issue_ **Compile-time failure: wrong `DefaultQuery` usage** `c.DefaultQuery("tag")` requires a default value parameter (`key, defaultValue string`). This fails to compile. ```diff - tag := c.DefaultQuery("tag") + tag := c.DefaultQuery("tag", "latest") ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion tag := c.DefaultQuery("tag", "latest") if tag != "" && !strings.Contains(imageRef, ":") && !strings.Contains(imageRef, "@") { imageRef = imageRef + ":" + tag } else if !strings.Contains(imageRef, ":") && !strings.Contains(imageRef, "@") { ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 397 to 401, the call to c.DefaultQuery("tag") is incorrect because DefaultQuery requires two parameters: the key and a default value. Fix this by providing a default value string as the second argument, for example c.DefaultQuery("tag", ""), to ensure it compiles correctly. ``` </details>  ✅ Addressed in commit 03b3c56

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 08:50:30 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 0

♻️ Duplicate comments (2)

src/imagetar.go (2)

106-111: ⚠️ Potential issue

Still constructing an incomplete remote.Descriptor – will panic later

Only Manifest is populated; fields like Digest, Size, internal lazy fetchers, etc. stay zero/nil. Calls such as desc.Image() or desc.ImageIndex() (used immediately below) will panic.
Cache the raw manifest separately and always call remote.Get, or fully hydrate the struct via remote.WithContext/remote.Get.
This was pointed out in a previous review and remains unfixed.

560-575: ⚠️ Potential issue

Batch tar layout is still broken – files collide across images

A directory entry (dirName) is emitted, but StreamImageToWriter writes all files at the archive root, so every image after the first overwrites manifest.json, config blobs, layer tar files, etc.
Pass the directory prefix down (e.g., wrap the tar.Writer with a path-prefixing writer or let streamDockerFormat accept a base path) to ensure each image’s files live under its own directory.
Previous review flagged the same issue.

🧹 Nitpick comments (1)

src/imagetar.go (1)

23-26: concurrency field is never used

ImageStreamer stores a configurable concurrency value, but no code leverages it (no worker pool, semaphore, etc.).
Either implement parallel pulls limited by this field or remove the dead configuration to avoid a misleading knob.

Also applies to: 39-47

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 80b3f8959f and 03b3c565b9.

📒 Files selected for processing (1)

src/imagetar.go (1 hunks)

**Actionable comments posted: 0** <details> <summary>♻️ Duplicate comments (2)</summary><blockquote> <details> <summary>src/imagetar.go (2)</summary><blockquote> `106-111`: _⚠️ Potential issue_ **Still constructing an incomplete `remote.Descriptor` – will panic later** Only `Manifest` is populated; fields like `Digest`, `Size`, internal lazy fetchers, etc. stay zero/nil. Calls such as `desc.Image()` or `desc.ImageIndex()` (used immediately below) will panic. Cache the raw manifest separately and always call `remote.Get`, or fully hydrate the struct via `remote.WithContext`/`remote.Get`. This was pointed out in a previous review and remains unfixed. --- `560-575`: _⚠️ Potential issue_ **Batch tar layout is still broken – files collide across images** A directory entry (`dirName`) is emitted, but `StreamImageToWriter` writes all files at the archive root, so every image after the first overwrites `manifest.json`, config blobs, layer tar files, etc. Pass the directory prefix down (e.g., wrap the `tar.Writer` with a path-prefixing writer or let `streamDockerFormat` accept a base path) to ensure each image’s files live under its own directory. Previous review flagged the same issue. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>src/imagetar.go (1)</summary><blockquote> `23-26`: **`concurrency` field is never used** `ImageStreamer` stores a configurable concurrency value, but no code leverages it (no worker pool, semaphore, etc.). Either implement parallel pulls limited by this field or remove the dead configuration to avoid a misleading knob. Also applies to: 39-47 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 80b3f8959ffbb169e6d35e29cdee2236b0521a24 and 03b3c565b9f39d5bfa36855d11a803051c939df4. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/imagetar.go` (1 hunks) </details> </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 09:12:50 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 0

♻️ Duplicate comments (2)

src/imagetar.go (2)
96-113: ⚠️ Potential issue

Cached remote.Descriptor is still incomplete – runtime panics will resurface

The shortcut path only fills Manifest; MediaType, Digest, Size, URLs, Client, etc. remain zero / nil.
StreamImageToWriter immediately inspects desc.MediaType, and later calls desc.Image() / desc.ImageIndex(), both of which rely on the missing internals and will panic or mis-route.
-		if reference != "" {
-			cacheKey := buildManifestCacheKey(ref.Context().String(), reference)
-					if cachedItem := globalCache.Get(cacheKey); cachedItem != nil {
-			desc := &remote.Descriptor{
-				Manifest: cachedItem.Data,
-			}
+		if reference != "" {
+			cacheKey := buildManifestCacheKey(ref.Context().String(), reference)
+			if cachedItem := globalCache.Get(cacheKey); cachedItem != nil {
+				// Re-hydrate via remote.WithContext instead of manual struct construction
+				desc, err := remote.Get(ref,
+					append(options, remote.WithContext(context.TODO()), remote.WithTransport(GetGlobalHTTPClient().Transport))...)
+				if err == nil {
+					log.Printf("使用缓存的manifest: %s", ref.String())
+					return desc, nil
+				}
Either always call remote.Get (letting CRI reuse ETag-based caching) or populate all required fields.
Issue was already flagged in a previous review and remains unresolved.

554-579: ⚠️ Potential issue

Batch tar layout still overwrites files – directory prefix unused

A directory header (image_%d_…/) is written, yet StreamImageToWriter writes all config, layer and manifest files at the tar root, causing collisions when multiple images share layer digests or config names. The earlier review comment about this exact issue has not been addressed.

Minimum fix: pipe a writer that automatically prefixes every header name with dirName, e.g.
prefixWriter := tarNewPrefixWriter(tarWriter, dirName) // custom io.Writer to rewrite hdr.Name
if err := is.StreamImageToWriter(ctx, imageRef, prefixWriter, &StreamOptions{...}); err != nil { ... }
or extend streamDockerFormat so it accepts a path prefix.

Until resolved, the produced archive is not usable for more than one image.

🧹 Nitpick comments (2)

src/imagetar.go (2)
23-27: concurrency field is dead code

ImageStreamer stores concurrency, yet no semaphore / worker pool leverages it—StreamMultipleImages loops serially. Either:

Implement concurrent downloads using errgroup.Group limited by concurrency, or

Drop the field to avoid misleading configuration.

This also affects advertised performance improvements in the PR description.

Also applies to: 40-48

260-264: Colon in filenames breaks Windows extraction

configDigest.String() includes the algorithm prefix (sha256:), producing names like sha256:abcd.json. The colon is illegal on Windows filesystems, so tools such as 7-Zip fail to extract.
-	Name: configDigest.String() + ".json",
+	Name: strings.ReplaceAll(configDigest.String(), ":", "_") + ".json",
Apply the same sanitisation to layer directory names if cross-platform compatibility matters.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 03b3c565b9 and e1920edbd7.

📒 Files selected for processing (1)

src/imagetar.go (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/imagetar.go (3)

src/config.go (1)

GetConfig (143-159)

src/http_client.go (1)

GetGlobalHTTPClient (52-54)

src/ratelimiter.go (1)

RateLimitMiddleware (219-278)

🔇 Additional comments (1)

src/imagetar.go (1)

281-310: Nice – immediate resource release fixes the FD leak

Wrapping the per-layer logic in an anonymous function and deferring layerReader.Close() inside it ensures each reader is closed before the next iteration. This eliminates the previously reported file-descriptor accumulation for large images.

**Actionable comments posted: 0** <details> <summary>♻️ Duplicate comments (2)</summary><blockquote> <details> <summary>src/imagetar.go (2)</summary><blockquote> `96-113`: _⚠️ Potential issue_ **Cached `remote.Descriptor` is still incomplete – runtime panics will resurface** The shortcut path only fills `Manifest`; `MediaType`, `Digest`, `Size`, `URLs`, `Client`, etc. remain zero / nil. `StreamImageToWriter` immediately inspects `desc.MediaType`, and later calls `desc.Image()` / `desc.ImageIndex()`, both of which rely on the missing internals and will panic or mis-route. ```diff - if reference != "" { - cacheKey := buildManifestCacheKey(ref.Context().String(), reference) - if cachedItem := globalCache.Get(cacheKey); cachedItem != nil { - desc := &remote.Descriptor{ - Manifest: cachedItem.Data, - } + if reference != "" { + cacheKey := buildManifestCacheKey(ref.Context().String(), reference) + if cachedItem := globalCache.Get(cacheKey); cachedItem != nil { + // Re-hydrate via remote.WithContext instead of manual struct construction + desc, err := remote.Get(ref, + append(options, remote.WithContext(context.TODO()), remote.WithTransport(GetGlobalHTTPClient().Transport))...) + if err == nil { + log.Printf("使用缓存的manifest: %s", ref.String()) + return desc, nil + } ``` Either always call `remote.Get` (letting CRI reuse ETag-based caching) or populate **all** required fields. Issue was already flagged in a previous review and remains unresolved. --- `554-579`: _⚠️ Potential issue_ **Batch tar layout still overwrites files – directory prefix unused** A directory header (`image_%d_…/`) is written, yet `StreamImageToWriter` writes all config, layer and manifest files at the tar root, causing collisions when multiple images share layer digests or config names. The earlier review comment about this exact issue has not been addressed. Minimum fix: pipe a writer that automatically prefixes every header name with `dirName`, e.g. ```go prefixWriter := tarNewPrefixWriter(tarWriter, dirName) // custom io.Writer to rewrite hdr.Name if err := is.StreamImageToWriter(ctx, imageRef, prefixWriter, &StreamOptions{...}); err != nil { ... } ``` or extend `streamDockerFormat` so it accepts a path prefix. Until resolved, the produced archive is not usable for more than one image. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>src/imagetar.go (2)</summary><blockquote> `23-27`: **`concurrency` field is dead code** `ImageStreamer` stores `concurrency`, yet no semaphore / worker pool leverages it—`StreamMultipleImages` loops serially. Either: 1. Implement concurrent downloads using `errgroup.Group` limited by `concurrency`, or 2. Drop the field to avoid misleading configuration. This also affects advertised performance improvements in the PR description. Also applies to: 40-48 --- `260-264`: **Colon in filenames breaks Windows extraction** `configDigest.String()` includes the algorithm prefix (`sha256:`), producing names like `sha256:abcd.json`. The colon is illegal on Windows filesystems, so tools such as 7-Zip fail to extract. ```diff - Name: configDigest.String() + ".json", + Name: strings.ReplaceAll(configDigest.String(), ":", "_") + ".json", ``` Apply the same sanitisation to layer directory names if cross-platform compatibility matters. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 03b3c565b9f39d5bfa36855d11a803051c939df4 and e1920edbd7a3cd863ec82c358fc46bee24072c2f. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/imagetar.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/imagetar.go (3)</summary><blockquote> <details> <summary>src/config.go (1)</summary> * `GetConfig` (143-159) </details> <details> <summary>src/http_client.go (1)</summary> * `GetGlobalHTTPClient` (52-54) </details> <details> <summary>src/ratelimiter.go (1)</summary> * `RateLimitMiddleware` (219-278) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>src/imagetar.go (1)</summary> `281-310`: **Nice – immediate resource release fixes the FD leak** Wrapping the per-layer logic in an anonymous function and deferring `layerReader.Close()` inside it ensures each reader is closed before the next iteration. This eliminates the previously reported file-descriptor accumulation for large images. </details> </blockquote></details> </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 10:30:53 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/imagetar.go (2)

24-27: concurrency field is never used

ImageStreamer stores a concurrency setting but no code paths consume it (all downloads are sequential).
Either wire this into a worker-pool / semaphore, or drop the field to avoid misleading configuration knobs.

685-735: Duplicate layer/config entries inflate batch tar

StreamMultipleImages writes every image’s layers and configs unconditionally.
When multiple images share the same digest (common for different tags), the archive will contain identical files multiple times, increasing size and time.

Consider tracking already-written digests and skipping duplicate writes to keep the tar lean.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0e7740c391 and 615aec79e6.

📒 Files selected for processing (2)

src/imagetar.go (1 hunks)
src/public/images.html (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/public/images.html

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/imagetar.go (3)

src/config.go (1)

GetConfig (143-159)

src/http_client.go (1)

GetGlobalHTTPClient (52-54)

src/ratelimiter.go (1)

RateLimitMiddleware (219-278)

**Actionable comments posted: 1** <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>src/imagetar.go (2)</summary><blockquote> `24-27`: **`concurrency` field is never used** `ImageStreamer` stores a `concurrency` setting but no code paths consume it (all downloads are sequential). Either wire this into a worker-pool / semaphore, or drop the field to avoid misleading configuration knobs. --- `685-735`: **Duplicate layer/config entries inflate batch tar** `StreamMultipleImages` writes every image’s layers and configs unconditionally. When multiple images share the same digest (common for different tags), the archive will contain identical files multiple times, increasing size and time. Consider tracking already-written digests and skipping duplicate writes to keep the tar lean. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0e7740c391dfefc9117ee72533d439725aeeabc5 and 615aec79e689e0caee6835a803abd1f4cbd7abd4. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `src/imagetar.go` (1 hunks) * `src/public/images.html` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/public/images.html </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/imagetar.go (3)</summary><blockquote> <details> <summary>src/config.go (1)</summary> * `GetConfig` (143-159) </details> <details> <summary>src/http_client.go (1)</summary> * `GetGlobalHTTPClient` (52-54) </details> <details> <summary>src/ratelimiter.go (1)</summary> * `RateLimitMiddleware` (219-278) </details> </blockquote></details> </details> </details> </details>

src/imagetar.go

						
				@@ -0,0 +1,932 @@

				package main

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 10:30:52 +08:00

⚠️ Potential issue

Cached descriptor is still incomplete – will panic downstream

Re-creating remote.Descriptor with only Manifest populated discards internal fetchers (Client, ref, etc.).
Later calls such as desc.Image() or desc.ImageIndex() will nil-panic, exactly the issue flagged in a previous review.

Please revert to always obtaining the descriptor via remote.Get, or re-inject the cached bytes through remote.WithContext/remote.Get instead of manual struct construction.

🤖 Prompt for AI Agents

In src/imagetar.go around lines 107 to 112, the code incorrectly reconstructs a
remote.Descriptor by only setting the Manifest field, which causes nil pointer
panics later because internal fetchers like Client and ref are missing. To fix
this, avoid manually creating the Descriptor struct from cached data; instead,
always obtain the descriptor by calling remote.Get or use remote.WithContext
combined with remote.Get to re-inject the cached manifest bytes properly,
ensuring the Descriptor is fully initialized with all necessary internal fields.

_⚠️ Potential issue_ **Cached descriptor is still incomplete – will panic downstream** Re-creating `remote.Descriptor` with only `Manifest` populated discards internal fetchers (`Client`, `ref`, etc.). Later calls such as `desc.Image()` or `desc.ImageIndex()` will nil-panic, exactly the issue flagged in a previous review. Please revert to always obtaining the descriptor via `remote.Get`, or re-inject the cached bytes through `remote.WithContext`/`remote.Get` instead of manual struct construction. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 107 to 112, the code incorrectly reconstructs a remote.Descriptor by only setting the Manifest field, which causes nil pointer panics later because internal fetchers like Client and ref are missing. To fix this, avoid manually creating the Descriptor struct from cached data; instead, always obtain the descriptor by calling remote.Get or use remote.WithContext combined with remote.Get to re-inject the cached manifest bytes properly, ensuring the Descriptor is fully initialized with all necessary internal fields. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 11:06:38 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 2

♻️ Duplicate comments (1)

src/imagetar.go (1)

111-118: ⚠️ Potential issue

Manual remote.Descriptor reconstruction is still broken

Previous reviews already explained that fabricating a descriptor with only Manifest set leaves essential fields (Digest, Size, internal client/fetchers) nil, leading to mis-detection of multi-arch images and later panics when Image() / ImageIndex() are called.

Please revert to obtaining the descriptor via remote.Get (and just cache the raw manifest bytes separately) or fully populate the struct.

🧹 Nitpick comments (2)

src/imagetar.go (2)

259-263: Colon in tar pathname may break Windows clients

layerDir := digest.String() yields sha256:abcd…, so the generated tar entry sha256:abcd…/layer.tar contains :, which is illegal on Windows file systems and uncommon in docker save archives.
Replace : with / or drop the algorithm prefix to mirror Docker’s layout (<digest>/layer.tar).

24-27: concurrency is never used

The field is stored and logged but not referenced elsewhere; actual streaming runs sequentially. Either remove the dead code or apply a worker-pool in StreamMultipleImages.

Also applies to: 40-48

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 615aec79e6 and 018a8235e2.

📒 Files selected for processing (2)

src/imagetar.go (1 hunks)
src/token_cache.go (1 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/imagetar.go (3)

src/config.go (1)

GetConfig (143-159)

src/http_client.go (1)

GetGlobalHTTPClient (52-54)

src/ratelimiter.go (1)

RateLimitMiddleware (219-278)

**Actionable comments posted: 2** <details> <summary>♻️ Duplicate comments (1)</summary><blockquote> <details> <summary>src/imagetar.go (1)</summary><blockquote> `111-118`: _⚠️ Potential issue_ **Manual `remote.Descriptor` reconstruction is still broken** Previous reviews already explained that fabricating a descriptor with only `Manifest` set leaves essential fields (`Digest`, `Size`, internal client/fetchers) nil, leading to mis-detection of multi-arch images and later panics when `Image()` / `ImageIndex()` are called. Please revert to obtaining the descriptor via `remote.Get` (and just cache the raw manifest bytes separately) or fully populate the struct. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>src/imagetar.go (2)</summary><blockquote> `259-263`: **Colon in tar pathname may break Windows clients** `layerDir := digest.String()` yields `sha256:abcd…`, so the generated tar entry `sha256:abcd…/layer.tar` contains `:`, which is illegal on Windows file systems and uncommon in `docker save` archives. Replace `:` with `/` or drop the algorithm prefix to mirror Docker’s layout (`<digest>/layer.tar`). --- `24-27`: **`concurrency` is never used** The field is stored and logged but not referenced elsewhere; actual streaming runs sequentially. Either remove the dead code or apply a worker-pool in `StreamMultipleImages`. Also applies to: 40-48 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 615aec79e689e0caee6835a803abd1f4cbd7abd4 and 018a8235e2903b08d35409401d1ed06aa2fb7cd0. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `src/imagetar.go` (1 hunks) * `src/token_cache.go` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/imagetar.go (3)</summary><blockquote> <details> <summary>src/config.go (1)</summary> * `GetConfig` (143-159) </details> <details> <summary>src/http_client.go (1)</summary> * `GetGlobalHTTPClient` (52-54) </details> <details> <summary>src/ratelimiter.go (1)</summary> * `RateLimitMiddleware` (219-278) </details> </blockquote></details> </details> </details> </details>

src/imagetar.go

						
				@@ -0,0 +608,4 @@

								   m.Platform.Variant == targetVariant {

									selectedDesc = &m

									break

								}

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 11:06:37 +08:00

🛠️ Refactor suggestion

Variant matching is too strict

If the caller omits a variant (arm64 vs arm64/v8), the equality check fails.
Skip the variant comparison when either side is empty to make platform selection more forgiving.

-   m.Platform.Variant == targetVariant
+   (targetVariant == "" || m.Platform.Variant == targetVariant)

📝 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.

				if m.Platform.OS == targetOS && 
				   m.Platform.Architecture == targetArch &&
				   (targetVariant == "" || m.Platform.Variant == targetVariant) {
					selectedDesc = &m
					break
				}

🤖 Prompt for AI Agents

In src/imagetar.go around lines 484 to 499, the current platform variant
comparison requires exact matches, causing failures when one side omits the
variant. Modify the condition to skip variant comparison if either the
targetVariant or m.Platform.Variant is empty, allowing selection to succeed if
OS and Architecture match and variant is either empty or equal.

_🛠️ Refactor suggestion_ **Variant matching is too strict** If the caller omits a variant (`arm64` vs `arm64/v8`), the equality check fails. Skip the variant comparison when either side is empty to make platform selection more forgiving. ```diff - m.Platform.Variant == targetVariant + (targetVariant == "" || m.Platform.Variant == targetVariant) ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion if m.Platform.OS == targetOS && m.Platform.Architecture == targetArch && (targetVariant == "" || m.Platform.Variant == targetVariant) { selectedDesc = &m break } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 484 to 499, the current platform variant comparison requires exact matches, causing failures when one side omits the variant. Modify the condition to skip variant comparison if either the targetVariant or m.Platform.Variant is empty, allowing selection to succeed if OS and Architecture match and variant is either empty or equal. ``` </details>

src/token_cache.go

						
				@@ -82,0 +75,4 @@

					if platform == "" {

						platform = "default"

					}

					key := fmt.Sprintf("%s:%s@%s", imageRef, reference, platform)

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 11:06:37 +08:00

🛠️ Refactor suggestion

Consider key-collision & legacy-cache alignment

Using "default" when platform == "" means the same manifest will now be cached twice (manifest:<hash> and manifest:<hash-of-…@default>).
Either migrate all callers to the new helper or keep the legacy key when platform == "" to avoid doubled memory and unpredictable cache hits.

-if platform == "" {
-    platform = "default"
-}
+// Preserve backward-compat – keep old key when platform unspecified
+if platform == "" {
+    return buildManifestCacheKey(imageRef, reference)
+}

📝 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.

// buildManifestCacheKeyWithPlatform 构建包含平台信息的manifest缓存key
func buildManifestCacheKeyWithPlatform(imageRef, reference, platform string) string {
    // Preserve backward-compat – keep old key when platform unspecified
    if platform == "" {
        return buildManifestCacheKey(imageRef, reference)
    }
    key := fmt.Sprintf("%s:%s@%s", imageRef, reference, platform)
    return buildCacheKey("manifest", key)
}

🤖 Prompt for AI Agents

In src/token_cache.go around lines 81 to 88, the function
buildManifestCacheKeyWithPlatform uses "default" as the platform string when
platform is empty, causing duplicate cache keys for the same manifest. To fix
this, modify the function so that when platform is empty, it returns the legacy
cache key format without appending "@default". This avoids key collisions and
redundant cache entries, ensuring consistent cache hits and memory usage.

_🛠️ Refactor suggestion_ **Consider key-collision & legacy-cache alignment** Using `"default"` when `platform == ""` means the *same* manifest will now be cached twice (`manifest:<hash>` and `manifest:<hash-of-…@default>`). Either migrate all callers to the new helper or keep the legacy key when `platform == ""` to avoid doubled memory and unpredictable cache hits. ```diff -if platform == "" { - platform = "default" -} +// Preserve backward-compat – keep old key when platform unspecified +if platform == "" { + return buildManifestCacheKey(imageRef, reference) +} ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion // buildManifestCacheKeyWithPlatform 构建包含平台信息的manifest缓存key func buildManifestCacheKeyWithPlatform(imageRef, reference, platform string) string { // Preserve backward-compat – keep old key when platform unspecified if platform == "" { return buildManifestCacheKey(imageRef, reference) } key := fmt.Sprintf("%s:%s@%s", imageRef, reference, platform) return buildCacheKey("manifest", key) } ``` </details>  <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/token_cache.go around lines 81 to 88, the function buildManifestCacheKeyWithPlatform uses "default" as the platform string when platform is empty, causing duplicate cache keys for the same manifest. To fix this, modify the function so that when platform is empty, it returns the legacy cache key format without appending "@default". This avoids key collisions and redundant cache entries, ensuring consistent cache hits and memory usage. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 12:00:36 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

♻️ Duplicate comments (3)

src/imagetar.go (3)
717-753: ⚠️ Potential issue

Batch tar layout will cause file name collisions.

When downloading multiple images, each image's files (config, layers) are written to the root of the tar archive without any directory structure to separate them. This will cause file name collisions when images share the same layer digests or config names.

Consider organizing each image under its own directory in the tar archive. For example:
// In streamSingleImageForBatch, create a directory for each image
+dirName := fmt.Sprintf("image_%d_%s", imageIndex, strings.ReplaceAll(imageRef, "/", "_"))
+dirHeader := &tar.Header{
+    Name:     dirName + "/",
+    Typeflag: tar.TypeDir,
+    Mode:     0755,
+}
+if err := tarWriter.WriteHeader(dirHeader); err != nil {
+    return nil, nil, err
+}

// Then modify streamDockerFormatWithReturn to accept a path prefix
// and prepend it to all file names written to the tar
114-118: ⚠️ Potential issue

Manually created descriptor will cause panics.

Creating a remote.Descriptor with only the Manifest field populated will cause nil pointer panics when methods like desc.Image() or desc.ImageIndex() are called later, as they rely on internal fetchers that are not initialized.

Instead of manually constructing the descriptor, always use remote.Get to obtain a properly initialized descriptor. If you need to cache the manifest, store it separately and use it to optimize the remote.Get call through custom transport options.

495-497: 🛠️ Refactor suggestion

Platform variant matching is too strict.

The current implementation requires exact variant matches, which will fail when the caller specifies a platform without a variant (e.g., "linux/arm64") but the manifest has a variant (e.g., "arm64/v8").

Make the variant comparison more flexible:
 if m.Platform.OS == targetOS && 
    m.Platform.Architecture == targetArch &&
-   m.Platform.Variant == targetVariant {
+   (targetVariant == "" || m.Platform.Variant == "" || m.Platform.Variant == targetVariant) {
     selectedDesc = &m
     break
 }

🧹 Nitpick comments (1)

src/search.go (1)

117-139: Improved cache eviction strategy.

The new approach of first removing expired entries before checking capacity is more efficient. The batch removal of 25% of entries also reduces the frequency of eviction operations.

Consider tracking entry timestamps in a sorted structure (e.g., using a heap or sorted slice) to ensure the oldest entries are removed during batch eviction, as map iteration order in Go is non-deterministic.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 018a8235e2 and 8f4fa093c9.

📒 Files selected for processing (6)

src/config.go (4 hunks)
src/docker.go (1 hunks)
src/imagetar.go (1 hunks)
src/main.go (5 hunks)
src/ratelimiter.go (2 hunks)
src/search.go (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/main.go

🧰 Additional context used

🪛 golangci-lint (1.64.8)

src/ratelimiter.go

195-195: SA4006: this value of entry is never used

(staticcheck)

🔇 Additional comments (4)

src/docker.go (1)

82-82: Good logging consolidation.

Replacing the direct print statement with a comment aligns with the centralized startup logging approach seen across other initialization functions.

src/ratelimiter.go (1)

95-95: Consistent logging cleanup.

Good to see the same logging consolidation pattern applied here.

src/config.go (2)

150-189: Well-implemented configuration caching with proper concurrency control.

The double-checked locking pattern is correctly implemented, reducing the overhead of deep copying the configuration on each call while maintaining thread safety.

197-200: Proper cache invalidation on configuration updates.

Good to see the cache being cleared when configuration changes to maintain consistency.

**Actionable comments posted: 1** <details> <summary>♻️ Duplicate comments (3)</summary><blockquote> <details> <summary>src/imagetar.go (3)</summary><blockquote> `717-753`: _⚠️ Potential issue_ **Batch tar layout will cause file name collisions.** When downloading multiple images, each image's files (config, layers) are written to the root of the tar archive without any directory structure to separate them. This will cause file name collisions when images share the same layer digests or config names. Consider organizing each image under its own directory in the tar archive. For example: ```diff // In streamSingleImageForBatch, create a directory for each image +dirName := fmt.Sprintf("image_%d_%s", imageIndex, strings.ReplaceAll(imageRef, "/", "_")) +dirHeader := &tar.Header{ + Name: dirName + "/", + Typeflag: tar.TypeDir, + Mode: 0755, +} +if err := tarWriter.WriteHeader(dirHeader); err != nil { + return nil, nil, err +} // Then modify streamDockerFormatWithReturn to accept a path prefix // and prepend it to all file names written to the tar ``` --- `114-118`: _⚠️ Potential issue_ **Manually created descriptor will cause panics.** Creating a `remote.Descriptor` with only the `Manifest` field populated will cause nil pointer panics when methods like `desc.Image()` or `desc.ImageIndex()` are called later, as they rely on internal fetchers that are not initialized. Instead of manually constructing the descriptor, always use `remote.Get` to obtain a properly initialized descriptor. If you need to cache the manifest, store it separately and use it to optimize the `remote.Get` call through custom transport options. --- `495-497`: _🛠️ Refactor suggestion_ **Platform variant matching is too strict.** The current implementation requires exact variant matches, which will fail when the caller specifies a platform without a variant (e.g., "linux/arm64") but the manifest has a variant (e.g., "arm64/v8"). Make the variant comparison more flexible: ```diff if m.Platform.OS == targetOS && m.Platform.Architecture == targetArch && - m.Platform.Variant == targetVariant { + (targetVariant == "" || m.Platform.Variant == "" || m.Platform.Variant == targetVariant) { selectedDesc = &m break } ``` </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>src/search.go (1)</summary><blockquote> `117-139`: **Improved cache eviction strategy.** The new approach of first removing expired entries before checking capacity is more efficient. The batch removal of 25% of entries also reduces the frequency of eviction operations. Consider tracking entry timestamps in a sorted structure (e.g., using a heap or sorted slice) to ensure the oldest entries are removed during batch eviction, as map iteration order in Go is non-deterministic. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 018a8235e2903b08d35409401d1ed06aa2fb7cd0 and 8f4fa093c955e0ccb09e134c43f6e49ce0da82b5. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `src/config.go` (4 hunks) * `src/docker.go` (1 hunks) * `src/imagetar.go` (1 hunks) * `src/main.go` (5 hunks) * `src/ratelimiter.go` (2 hunks) * `src/search.go` (1 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/main.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> <details> <summary>src/ratelimiter.go</summary> 195-195: SA4006: this value of `entry` is never used (staticcheck) </details> </details> </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>src/docker.go (1)</summary> `82-82`: **Good logging consolidation.** Replacing the direct print statement with a comment aligns with the centralized startup logging approach seen across other initialization functions. </details> <details> <summary>src/ratelimiter.go (1)</summary> `95-95`: **Consistent logging cleanup.** Good to see the same logging consolidation pattern applied here. </details> <details> <summary>src/config.go (2)</summary> `150-189`: **Well-implemented configuration caching with proper concurrency control.** The double-checked locking pattern is correctly implemented, reducing the overhead of deep copying the configuration on each call while maintaining thread safety. --- `197-200`: **Proper cache invalidation on configuration updates.** Good to see the cache being cleared when configuration changes to maintain consistency. </details> </blockquote></details> </details>

src/ratelimiter.go

						
				@@ -193,3 +185,4 @@

					i.mu.RLock()

					entry, exists := i.ips[cleanIP]

					i.mu.RUnlock()

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 12:00:36 +08:00

⚠️ Potential issue

Remove unused variable assignment to avoid confusion.

The static analysis correctly identifies that the entry variable assigned at line 195 is never used because it's shadowed by the redeclaration at line 201. While this doesn't cause a functional issue, it could confuse future maintainers.

 // ✅ 双重检查锁定，解决竞态条件
 i.mu.RLock()
-entry, exists := i.ips[cleanIP]
+_, exists := i.ips[cleanIP]
 i.mu.RUnlock()
 
 if exists {
     // 安全更新访问时间
     i.mu.Lock()
     if entry, stillExists := i.ips[cleanIP]; stillExists {
         entry.lastAccess = now
         i.mu.Unlock()
         return entry.limiter, true
     }
     i.mu.Unlock()
 }

📝 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.

    now := time.Now()
    
    // ✅ 双重检查锁定，解决竞态条件
    i.mu.RLock()
    _, exists := i.ips[cleanIP]
    i.mu.RUnlock()
    
    if exists {
        // 安全更新访问时间
        i.mu.Lock()
        if entry, stillExists := i.ips[cleanIP]; stillExists {
            entry.lastAccess = now
            i.mu.Unlock()
            return entry.limiter, true
        }
        i.mu.Unlock()
    }

🧰 Tools

🪛 golangci-lint (1.64.8)

195-195: SA4006: this value of entry is never used

(staticcheck)

🤖 Prompt for AI Agents

In src/ratelimiter.go around lines 191 to 207, the variable 'entry' is assigned
at line 195 but then shadowed by a new declaration at line 201, making the first
assignment unused and potentially confusing. To fix this, remove the initial
assignment of 'entry' at line 195 and only declare it once inside the locked
section where it is actually used, ensuring no variable shadowing occurs.

_⚠️ Potential issue_ **Remove unused variable assignment to avoid confusion.** The static analysis correctly identifies that the `entry` variable assigned at line 195 is never used because it's shadowed by the redeclaration at line 201. While this doesn't cause a functional issue, it could confuse future maintainers. ```diff // ✅ 双重检查锁定，解决竞态条件 i.mu.RLock() -entry, exists := i.ips[cleanIP] +_, exists := i.ips[cleanIP] i.mu.RUnlock() if exists { // 安全更新访问时间 i.mu.Lock() if entry, stillExists := i.ips[cleanIP]; stillExists { entry.lastAccess = now i.mu.Unlock() return entry.limiter, true } i.mu.Unlock() } ```  <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion now := time.Now() // ✅ 双重检查锁定，解决竞态条件 i.mu.RLock() _, exists := i.ips[cleanIP] i.mu.RUnlock() if exists { // 安全更新访问时间 i.mu.Lock() if entry, stillExists := i.ips[cleanIP]; stillExists { entry.lastAccess = now i.mu.Unlock() return entry.limiter, true } i.mu.Unlock() } ``` </details>  <details> <summary>🧰 Tools</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> 195-195: SA4006: this value of `entry` is never used (staticcheck) </details> </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/ratelimiter.go around lines 191 to 207, the variable 'entry' is assigned at line 195 but then shadowed by a new declaration at line 201, making the first assignment unused and potentially confusing. To fix this, remove the initial assignment of 'entry' at line 195 and only declare it once inside the locked section where it is actually used, ensuring no variable shadowing occurs. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 12:09:56 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/main.go (1)
243-249: ⚠️ Potential issue

Fallback still uses an already-consumed resp.Body

ProcessSmart fully reads and closes resp.Body.
If it returns an error, the fallback path re-uses resp.Body, which is now drained/closed, so the client receives an empty (or partial) response.
-processedBody = resp.Body
-processedSize = 0
+// Re-issue the upstream request to obtain a fresh body,
+// or buffer the original body before calling ProcessSmart.
+newReq, _ := http.NewRequest(c.Request.Method, u, nil)
+newResp, rerr := GetGlobalHTTPClient().Do(newReq)
+if rerr == nil {
+    processedBody = newResp.Body
+} else {
+    c.String(http.StatusBadGateway, "upstream retry failed")
+    return
+}
+processedSize = 0
Alternatively, read resp.Body into a bytes.Buffer before passing it to ProcessSmart (use io.TeeReader) and keep the buffer for fallback.

🧹 Nitpick comments (2)

src/main.go (2)
75-83: Double recovery middleware – consider gin.New() instead of stacking

gin.Default() already installs its own Recovery middleware.
Adding another via router.Use(gin.CustomRecovery(...)) means two recoveries will run for every panic, causing duplicate logging and a small performance hit.

If a custom recovery is needed, start with gin.New() and install gin.Logger() + your custom recovery explicitly.

124-127: Prefer structured logging over fmt.Printf for startup info

Using the standard logger (or a structured logger such as log/slog/zap) provides timestamps and levels and integrates with Docker/K8s log collectors better than raw fmt.Printf.

Example:
log.Printf("🚀 HubProxy 启动成功 | addr=%s:%d rate=%d/%gh",
    cfg.Server.Host, cfg.Server.Port,
    cfg.RateLimit.RequestLimit, cfg.RateLimit.PeriodHours)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f4fa093c9 and fa14a5f5be.

📒 Files selected for processing (1)

src/main.go (7 hunks)

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/main.go (3)

src/proxysh.go (1)

ProcessSmart (16-43)

src/config.go (1)

GetConfig (149-189)

src/http_client.go (1)

GetGlobalHTTPClient (52-54)

🔇 Additional comments (1)

src/main.go (1)

320-387: Health & readiness endpoints look solid

Clear separation of /health (liveness) and /ready (readiness) with detailed component checks is excellent. 👍

**Actionable comments posted: 1** <details> <summary>♻️ Duplicate comments (1)</summary><blockquote> <details> <summary>src/main.go (1)</summary><blockquote> `243-249`: _⚠️ Potential issue_ **Fallback still uses an already-consumed `resp.Body`** `ProcessSmart` fully reads **and closes** `resp.Body`. If it returns an error, the fallback path re-uses `resp.Body`, which is now drained/closed, so the client receives an empty (or partial) response. ```diff -processedBody = resp.Body -processedSize = 0 +// Re-issue the upstream request to obtain a fresh body, +// or buffer the original body before calling ProcessSmart. +newReq, _ := http.NewRequest(c.Request.Method, u, nil) +newResp, rerr := GetGlobalHTTPClient().Do(newReq) +if rerr == nil { + processedBody = newResp.Body +} else { + c.String(http.StatusBadGateway, "upstream retry failed") + return +} +processedSize = 0 ``` Alternatively, read `resp.Body` into a `bytes.Buffer` before passing it to `ProcessSmart` (use `io.TeeReader`) and keep the buffer for fallback. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>src/main.go (2)</summary><blockquote> `75-83`: **Double recovery middleware – consider `gin.New()` instead of stacking** `gin.Default()` already installs its own `Recovery` middleware. Adding another via `router.Use(gin.CustomRecovery(...))` means two recoveries will run for every panic, causing duplicate logging and a small performance hit. If a custom recovery is needed, start with `gin.New()` and install `gin.Logger()` + your custom recovery explicitly. --- `124-127`: **Prefer structured logging over `fmt.Printf` for startup info** Using the standard logger (or a structured logger such as `log/slog`/`zap`) provides timestamps and levels and integrates with Docker/K8s log collectors better than raw `fmt.Printf`. Example: ```go log.Printf("🚀 HubProxy 启动成功 | addr=%s:%d rate=%d/%gh", cfg.Server.Host, cfg.Server.Port, cfg.RateLimit.RequestLimit, cfg.RateLimit.PeriodHours) ``` </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8f4fa093c955e0ccb09e134c43f6e49ce0da82b5 and fa14a5f5be1be67fe9438213b184c180d5af8cce. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `src/main.go` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/main.go (3)</summary><blockquote> <details> <summary>src/proxysh.go (1)</summary> * `ProcessSmart` (16-43) </details> <details> <summary>src/config.go (1)</summary> * `GetConfig` (149-189) </details> <details> <summary>src/http_client.go (1)</summary> * `GetGlobalHTTPClient` (52-54) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>src/main.go (1)</summary> `320-387`: **Health & readiness endpoints look solid** Clear separation of `/health` (liveness) and `/ready` (readiness) with detailed component checks is excellent. 👍 </details> </blockquote></details> </details>

src/main.go

						
				@@ -266,0 +333,4 @@

							checks["config"] = "failed"

							allReady = false

						}

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 12:09:56 +08:00

🛠️ Refactor suggestion

Trusting X-Forwarded-Host enables content-injection

realHost is taken verbatim from the client-controlled X-Forwarded-Host/Host header and fed into ProcessSmart, which rewrites GitHub URLs with that value.
An attacker can set X-Forwarded-Host: evil.com and have every rewritten URL point to https://evil.com/..., effectively turning the proxy into an open redirect / phishing vector.

Sanitise or ignore this header and use a server-side constant (e.g. from config) when rewriting URLs.

[security]

🤖 Prompt for AI Agents

In src/main.go around lines 228 to 235, the code trusts the client-controlled
X-Forwarded-Host header to set realHost, which can lead to content injection and
open redirect vulnerabilities. To fix this, remove the usage of X-Forwarded-Host
and instead use a server-side constant or configuration value for the host when
rewriting URLs. This ensures that the host used in URL rewriting is trusted and
not manipulable by clients.

_🛠️ Refactor suggestion_ **Trusting `X-Forwarded-Host` enables content-injection** `realHost` is taken verbatim from the client-controlled `X-Forwarded-Host`/`Host` header and fed into `ProcessSmart`, which rewrites GitHub URLs with that value. An attacker can set `X-Forwarded-Host: evil.com` and have every rewritten URL point to `https://evil.com/...`, effectively turning the proxy into an open redirect / phishing vector. Sanitise or ignore this header and use a server-side constant (e.g. from config) when rewriting URLs. [security] <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/main.go around lines 228 to 235, the code trusts the client-controlled X-Forwarded-Host header to set realHost, which can lead to content injection and open redirect vulnerabilities. To fix this, remove the usage of X-Forwarded-Host and instead use a server-side constant or configuration value for the host when rewriting URLs. This ensures that the host used in URL rewriting is trusted and not manipulable by clients. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 13:00:12 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 1

♻️ Duplicate comments (3)

src/imagetar.go (3)
596-609: Platform variant matching is still too strict

Requiring exact Variant equality breaks on common cases like arm64 vs arm64/v8.
Skip variant comparison when either side is empty to make selection tolerant:
-   m.Platform.Variant == targetVariant
+   (targetVariant == "" || m.Platform.Variant == targetVariant)
225-229: ⚠️ Potential issue

Cached remote.Descriptor is still incomplete – panic risk remains

Manually constructing a remote.Descriptor with only Manifest populated drops critical fields (Digest, Size, lazy fetchers, etc.).
Any subsequent call such as desc.Image()/desc.ImageIndex() will nil-panic – exactly the issue flagged in earlier reviews.
-desc := &remote.Descriptor{
-    Manifest: cachedItem.Data,
-}
+// ALWAYS obtain a fully-populated descriptor
+desc, err := remote.Get(ref, options...)
+if err != nil {
+    return nil, err
+}
+// ..then write desc.Manifest to cache if desired
343-355: ⚠️ Potential issue

Batch tar still writes every image at the archive root – name collisions inevitable

streamDockerFormatWithReturn emits configDigest.json, layerDir/layer.tar, etc. directly into the tar writer.
When StreamMultipleImages calls this for each image, later images overwrite earlier ones if any digest repeats (common with shared layers) or if two configs have identical filenames.

Provide each image with a unique prefix (e.g. sanitised ref or image_<idx>/) or open a new tar writer per sub-directory.
-func (is *ImageStreamer) streamDockerFormatWithReturn(..., imageRef string, ...) {
+func (is *ImageStreamer) streamDockerFormatWithReturn(..., imageRef string, idx int, ...) {
     prefix := fmt.Sprintf("image_%d/", idx)
-    configHeader := &tar.Header{
-        Name: configDigest.String() + ".json",
+    configHeader := &tar.Header{
+        Name: prefix + configDigest.String() + ".json",
     ...
 }

🧹 Nitpick comments (1)

src/imagetar.go (1)

137-139: concurrency field is dead code

ImageStreamer.concurrency is set but never used.
Either wire it into a worker-pool/semaphore or drop the field to avoid confusion.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa14a5f5be and 4756ada922.

📒 Files selected for processing (2)

src/imagetar.go (1 hunks)
src/main.go (7 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/main.go

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/imagetar.go (3)

src/config.go (1)

GetConfig (149-189)

src/http_client.go (1)

GetGlobalHTTPClient (52-54)

src/ratelimiter.go (1)

RateLimitMiddleware (229-288)

**Actionable comments posted: 1** <details> <summary>♻️ Duplicate comments (3)</summary><blockquote> <details> <summary>src/imagetar.go (3)</summary><blockquote> `596-609`: **Platform variant matching is still too strict** Requiring exact `Variant` equality breaks on common cases like `arm64` vs `arm64/v8`. Skip variant comparison when either side is empty to make selection tolerant: ```diff - m.Platform.Variant == targetVariant + (targetVariant == "" || m.Platform.Variant == targetVariant) ``` --- `225-229`: _⚠️ Potential issue_ **Cached `remote.Descriptor` is still incomplete – panic risk remains** Manually constructing a `remote.Descriptor` with only `Manifest` populated drops critical fields (`Digest`, `Size`, lazy fetchers, etc.). Any subsequent call such as `desc.Image()`/`desc.ImageIndex()` will nil-panic – exactly the issue flagged in earlier reviews. ```diff -desc := &remote.Descriptor{ - Manifest: cachedItem.Data, -} +// ALWAYS obtain a fully-populated descriptor +desc, err := remote.Get(ref, options...) +if err != nil { + return nil, err +} +// ..then write desc.Manifest to cache if desired ``` --- `343-355`: _⚠️ Potential issue_ **Batch tar still writes every image at the archive root – name collisions inevitable** `streamDockerFormatWithReturn` emits `configDigest.json`, `layerDir/layer.tar`, etc. directly into the tar writer. When `StreamMultipleImages` calls this for each image, later images overwrite earlier ones if any digest repeats (common with shared layers) or if two configs have identical filenames. Provide each image with a unique prefix (e.g. sanitised ref or `image_<idx>/`) or open a new tar writer per sub-directory. ```diff -func (is *ImageStreamer) streamDockerFormatWithReturn(..., imageRef string, ...) { +func (is *ImageStreamer) streamDockerFormatWithReturn(..., imageRef string, idx int, ...) { prefix := fmt.Sprintf("image_%d/", idx) - configHeader := &tar.Header{ - Name: configDigest.String() + ".json", + configHeader := &tar.Header{ + Name: prefix + configDigest.String() + ".json", ... } ``` </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>src/imagetar.go (1)</summary><blockquote> `137-139`: **`concurrency` field is dead code** `ImageStreamer.concurrency` is set but never used. Either wire it into a worker-pool/semaphore or drop the field to avoid confusion. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fa14a5f5be1be67fe9438213b184c180d5af8cce and 4756ada92240e0738adee6cf769bb3a593404abf. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `src/imagetar.go` (1 hunks) * `src/main.go` (7 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (1)</summary> * src/main.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/imagetar.go (3)</summary><blockquote> <details> <summary>src/config.go (1)</summary> * `GetConfig` (149-189) </details> <details> <summary>src/http_client.go (1)</summary> * `GetGlobalHTTPClient` (52-54) </details> <details> <summary>src/ratelimiter.go (1)</summary> * `RateLimitMiddleware` (229-288) </details> </blockquote></details> </details> </details> </details>

src/imagetar.go

						
				@@ -0,0 +705,4 @@

						log.Printf("镜像下载失败: %v", err)

						c.JSON(http.StatusInternalServerError, gin.H{"error": "镜像下载失败: " + err.Error()})

						return

					}

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 13:00:11 +08:00

🛠️ Refactor suggestion

Cannot send JSON after streaming has begun

If StreamImageToGin / StreamMultipleImages already wrote part of the response body, the subsequent c.JSON(...) call will fail or corrupt output.
Instead, log the error and simply abort the context; clients will observe a broken stream.

Also applies to: 767-770

🤖 Prompt for AI Agents

In src/imagetar.go around lines 704 to 708, the code attempts to send a JSON
error response after streaming has started, which can corrupt the output. To fix
this, remove the c.JSON call and instead log the error and call c.Abort() to
terminate the context without writing additional response data. Apply the same
fix to lines 767 to 770 where a similar pattern occurs.

_🛠️ Refactor suggestion_ **Cannot send JSON after streaming has begun** If `StreamImageToGin` / `StreamMultipleImages` already wrote part of the response body, the subsequent `c.JSON(...)` call will fail or corrupt output. Instead, log the error and simply abort the context; clients will observe a broken stream. Also applies to: 767-770 <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 704 to 708, the code attempts to send a JSON error response after streaming has started, which can corrupt the output. To fix this, remove the c.JSON call and instead log the error and call c.Abort() to terminate the context without writing additional response data. Apply the same fix to lines 767 to 770 where a similar pattern occurs. ``` </details>

coderabbitai[bot] (Migrated from github.com) reviewed 2025-06-13 14:03:44 +08:00

coderabbitai[bot] (Migrated from github.com) left a comment

Actionable comments posted: 3

♻️ Duplicate comments (2)

src/imagetar.go (2)

387-392: Close layer readers immediately (duplicate of previous comment)

defer layerReader.Close() inside the loop keeps every layer file open until the function returns.
Close it right after the io.Copy to avoid FD exhaustion on large images.

767-770: Cannot send JSON after streaming has begun (duplicate)

If StreamMultipleImages has written any bytes, the subsequent c.JSON corrupts the response.
Log the error and c.Abort() instead.

🧹 Nitpick comments (4)

src/public/index.html (1)

736-740: Don’t clobber global window.onclick

window.onclick = (…) overwrites any existing handler. Prefer
window.addEventListener('click', handler) to compose listeners safely.
src/imagetar.go (2)
596-608: Relax variant matching for better platform fallback

Exact equality on Variant rejects valid requests such as arm64 vs arm64/v8.
Skip the variant check when either side is empty:
- m.Platform.Variant == targetVariant
+ (targetVariant == "" || m.Platform.Variant == targetVariant)
70-84: Perform cleanup outside the hot lock

cleanup() iterates the entire map while the write-lock is held, blocking all incoming requests.
Consider unlocking first and running cleanup asynchronously, or use a separate goroutine.
src/public/search.html (1)

1281-1312: Extract common JS for theme & mobile-menu to a shared file

Identical blocks now live in index.html, search.html, and likely other pages.
Moving them to common.js (and shared CSS) will cut duplication and ease maintenance.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4756ada922 and 8ffceb7f2b.

📒 Files selected for processing (14)

README.md (1 hunks)
src/access_control.go (2 hunks)
src/config.go (2 hunks)
src/docker.go (1 hunks)
src/imagetar.go (1 hunks)
src/main.go (6 hunks)
src/proxysh.go (1 hunks)
src/public/images.html (1 hunks)
src/public/index.html (2 hunks)
src/public/search.html (25 hunks)
src/ratelimiter.go (4 hunks)
src/search.go (1 hunks)
src/smart_ratelimit.go (2 hunks)
src/token_cache.go (3 hunks)

✅ Files skipped from review due to trivial changes (2)

src/access_control.go
src/smart_ratelimit.go

🚧 Files skipped from review as they are similar to previous changes (8)

src/docker.go
src/config.go
README.md
src/search.go
src/token_cache.go
src/main.go
src/public/images.html
src/proxysh.go

🧰 Additional context used

🧬 Code Graph Analysis (1)

src/imagetar.go (3)

src/config.go (1)

GetConfig (146-185)

src/http_client.go (1)

GetGlobalHTTPClient (52-54)

src/ratelimiter.go (1)

RateLimitMiddleware (218-276)

🪛 golangci-lint (1.64.8)

src/ratelimiter.go

187-187: SA4006: this value of entry is never used

(staticcheck)

**Actionable comments posted: 3** <details> <summary>♻️ Duplicate comments (2)</summary><blockquote> <details> <summary>src/imagetar.go (2)</summary><blockquote> `387-392`: **Close layer readers immediately (duplicate of previous comment)** `defer layerReader.Close()` inside the loop keeps every layer file open until the function returns. Close it right after the `io.Copy` to avoid FD exhaustion on large images. --- `767-770`: **Cannot send JSON after streaming has begun (duplicate)** If `StreamMultipleImages` has written any bytes, the subsequent `c.JSON` corrupts the response. Log the error and `c.Abort()` instead. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>src/public/index.html (1)</summary><blockquote> `736-740`: **Don’t clobber global `window.onclick`** `window.onclick = (…)` overwrites any existing handler. Prefer `window.addEventListener('click', handler)` to compose listeners safely. </blockquote></details> <details> <summary>src/imagetar.go (2)</summary><blockquote> `596-608`: **Relax variant matching for better platform fallback** Exact equality on `Variant` rejects valid requests such as `arm64` vs `arm64/v8`. Skip the variant check when either side is empty: ```diff - m.Platform.Variant == targetVariant + (targetVariant == "" || m.Platform.Variant == targetVariant) ``` --- `70-84`: **Perform cleanup outside the hot lock** `cleanup()` iterates the entire map while the write-lock is held, blocking all incoming requests. Consider unlocking first and running cleanup asynchronously, or use a separate goroutine. </blockquote></details> <details> <summary>src/public/search.html (1)</summary><blockquote> `1281-1312`: **Extract common JS for theme & mobile-menu to a shared file** Identical blocks now live in `index.html`, `search.html`, and likely other pages. Moving them to `common.js` (and shared CSS) will cut duplication and ease maintenance. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 4756ada92240e0738adee6cf769bb3a593404abf and 8ffceb7f2b6f4f9ea47833d64aa952018f70f4d9. </details> <details> <summary>📒 Files selected for processing (14)</summary> * `README.md` (1 hunks) * `src/access_control.go` (2 hunks) * `src/config.go` (2 hunks) * `src/docker.go` (1 hunks) * `src/imagetar.go` (1 hunks) * `src/main.go` (6 hunks) * `src/proxysh.go` (1 hunks) * `src/public/images.html` (1 hunks) * `src/public/index.html` (2 hunks) * `src/public/search.html` (25 hunks) * `src/ratelimiter.go` (4 hunks) * `src/search.go` (1 hunks) * `src/smart_ratelimit.go` (2 hunks) * `src/token_cache.go` (3 hunks) </details> <details> <summary>✅ Files skipped from review due to trivial changes (2)</summary> * src/access_control.go * src/smart_ratelimit.go </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (8)</summary> * src/docker.go * src/config.go * README.md * src/search.go * src/token_cache.go * src/main.go * src/public/images.html * src/proxysh.go </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/imagetar.go (3)</summary><blockquote> <details> <summary>src/config.go (1)</summary> * `GetConfig` (146-185) </details> <details> <summary>src/http_client.go (1)</summary> * `GetGlobalHTTPClient` (52-54) </details> <details> <summary>src/ratelimiter.go (1)</summary> * `RateLimitMiddleware` (218-276) </details> </blockquote></details> </details><details> <summary>🪛 golangci-lint (1.64.8)</summary> <details> <summary>src/ratelimiter.go</summary> 187-187: SA4006: this value of `entry` is never used (staticcheck) </details> </details> </details> </details>

src/imagetar.go

						
				@@ -0,0 +227,4 @@

								}

								log.Printf("使用缓存的manifest: %s (平台: %s)", ref.String(), platform)

								return desc, nil

							}

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 14:03:42 +08:00

⚠️ Potential issue

Cached descriptor is still incomplete – will panic downstream

Creating a remote.Descriptor{ Manifest: … } omits mandatory fields and internal fetchers.
Subsequent calls (desc.Image(), desc.ImageIndex()) nil-panic.

Store the raw manifest separately but always obtain a full descriptor via remote.Get, or hydrate the cached bytes with remote.WithContext + remote.Get. Never hand-craft the struct.

🤖 Prompt for AI Agents

In src/imagetar.go around lines 225 to 230, the code creates a remote.Descriptor
struct manually with only the Manifest field, which lacks mandatory fields and
internal fetchers, causing nil panics later. To fix this, do not construct the
descriptor by hand; instead, store the raw manifest bytes separately and always
obtain a complete descriptor by calling remote.Get or by hydrating the cached
bytes using remote.WithContext combined with remote.Get. Replace the manual
struct creation with this approach to ensure the descriptor is fully populated
and safe to use downstream.

_⚠️ Potential issue_ **Cached descriptor is still incomplete – will panic downstream** Creating a `remote.Descriptor{ Manifest: … }` omits mandatory fields and internal fetchers. Subsequent calls (`desc.Image()`, `desc.ImageIndex()`) nil-panic. Store the raw manifest separately but always obtain a full descriptor via `remote.Get`, or hydrate the cached bytes with `remote.WithContext` + `remote.Get`. Never hand-craft the struct. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/imagetar.go around lines 225 to 230, the code creates a remote.Descriptor struct manually with only the Manifest field, which lacks mandatory fields and internal fetchers, causing nil panics later. To fix this, do not construct the descriptor by hand; instead, store the raw manifest bytes separately and always obtain a complete descriptor by calling remote.Get or by hydrating the cached bytes using remote.WithContext combined with remote.Get. Replace the manual struct creation with this approach to ensure the descriptor is fully populated and safe to use downstream. ``` </details>

src/public/index.html

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 14:03:42 +08:00

⚠️ Potential issue

DOMContentLoaded handler never fires when the script is at page-bottom

Because the script is executed after the DOM is already parsed, DOMContentLoaded has already been dispatched and the callback that sets up the modal / domain spans will never run.
Call the setup routine immediately (or wrap the whole script in an IIFE) instead of registering a late listener.

🤖 Prompt for AI Agents

In src/public/index.html around lines 726 to 742, the DOMContentLoaded event
listener does not fire because the script runs after the DOM is fully parsed. To
fix this, remove the DOMContentLoaded event listener and immediately execute the
setup code instead, either by placing the code directly or wrapping it in an
immediately invoked function expression (IIFE) so the modal and domain span
setup runs as soon as the script loads.

_⚠️ Potential issue_ **DOMContentLoaded handler never fires when the script is at page-bottom** Because the script is executed after the DOM is already parsed, `DOMContentLoaded` has already been dispatched and the callback that sets up the modal / domain spans will never run. Call the setup routine immediately (or wrap the whole script in an IIFE) instead of registering a late listener. <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/public/index.html around lines 726 to 742, the DOMContentLoaded event listener does not fire because the script runs after the DOM is fully parsed. To fix this, remove the DOMContentLoaded event listener and immediately execute the setup code instead, either by placing the code directly or wrapping it in an immediately invoked function expression (IIFE) so the modal and domain span setup runs as soon as the script loads. ``` </details>

src/ratelimiter.go

coderabbitai[bot] (Migrated from github.com) commented

2025-06-13 14:03:42 +08:00

🛠️ Refactor suggestion

Remove the first entry assignment – it’s dead code.

entry obtained under the initial RLock is never read because the subsequent i.mu.Lock() block redeclares/shadows it.
Drop the redundant read-lock lookup (or return early straight after it) to avoid the SA4006 warning and make the double-checked locking easier to follow.

🧰 Tools

🪛 golangci-lint (1.64.8)

187-187: SA4006: this value of entry is never used

(staticcheck)

🤖 Prompt for AI Agents

In src/ratelimiter.go around lines 186 to 195, remove the initial assignment of
`entry` under the read lock since it is shadowed and unused later. Instead,
perform only the read lock check for existence of `cleanIP` in `i.ips` and
return early if not found. Then acquire the write lock and re-check the map to
update and return the entry. This eliminates the dead code and clarifies the
double-checked locking pattern.

_🛠️ Refactor suggestion_ **Remove the first `entry` assignment – it’s dead code.** `entry` obtained under the initial `RLock` is never read because the subsequent `i.mu.Lock()` block redeclares/shadows it. Drop the redundant read-lock lookup (or return early straight after it) to avoid the SA4006 warning and make the double-checked locking easier to follow. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 golangci-lint (1.64.8)</summary> 187-187: SA4006: this value of `entry` is never used (staticcheck) </details> </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` In src/ratelimiter.go around lines 186 to 195, remove the initial assignment of `entry` under the read lock since it is shadowed and unused later. Instead, perform only the read lock check for existence of `cleanIP` in `i.ips` and return early if not found. Then acquire the write lock and re-check the map to update and return the entry. This eliminates the dead code and clarifies the double-checked locking pattern. ``` </details>

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: 3344/hubproxy#13

				`@@ -1,7 +1,7 @@`
				`package main`

				`@@ -0,0 +1,932 @@`
				`package main`

				`@@ -0,0 +1,932 @@`
				`package main`

🎉 v1.1.0 #13

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

Poem

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

CodeRabbit Configuration File (`.coderabbit.yaml`)