NO-JIRA: Add a singleton instance for list of nodes#30830
NO-JIRA: Add a singleton instance for list of nodes#30830sgoveas wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a thread-safe, lazy singleton that reads and parses Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Singleton as RDU2HostsSingleton
participant FS as FileSystem
participant YAML as YAMLParser
Caller->>Singleton: Call GetRDU2HostsList()
alt singleton uninitialized
Singleton->>Singleton: lock singletonMu
Singleton->>FS: read SHARED_DIR/hosts.yaml
FS-->>Singleton: file contents
Singleton->>YAML: unmarshal YAML
YAML-->>Singleton: []RDU2Host
Singleton->>Singleton: validate entries (non-empty, unique names)
Singleton->>Singleton: build hostsMap
Singleton-->>Singleton: unlock singletonMu
end
Singleton->>Singleton: RLock hostsMap.mu
Singleton-->>Caller: return defensive deep copy of hosts map
Singleton->>Singleton: RUnlock hostsMap.mu
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 65-68: The loop that builds hostsMap (using hostsMap, hostsData,
RDU2Host) currently allows empty names and silently overwrites duplicates;
update the loop to validate each hostsData[i].Name is non-empty and to detect
duplicates before insertion, returning or propagating an error (or logging and
skipping as appropriate to callsite policy) when name == "" or when hostsMap
already contains that name so callers cannot accidentally select the wrong host
later.
- Around line 90-97: The current copy (hostsCopy) only copies pointer references
from rdu2HostsSingleton.hostsMap so callers can still mutate shared *RDU2Host
objects; instead perform a deep copy of each RDU2Host value when building
hostsCopy: while holding rdu2HostsSingleton.mu.RLock()/RUnlock() iterate
rdu2HostsSingleton.hostsMap and for each entry allocate a new RDU2Host and copy
all fields (or call a Clone/Copy method on RDU2Host if one exists) so hostsCopy
contains independent copies and cannot be mutated by callers to affect the
original map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1a0e232-ea64-4237-ba51-5b15af67c0ba
📒 Files selected for processing (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go
569a7ae to
166b309
Compare
|
@sgoveas: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rioliu-rh please review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go (1)
85-99: Reduce lock scope inGetRDU2HostsListto avoid global serialization.Line 85 keeps
singletonMulocked through the full map copy path, so every caller is serialized even after initialization. Lock only for initialization, then copy underrdu2HostsSingleton.muonly.Proposed refactor
func GetRDU2HostsList() (map[string]*RDU2Host, error) { - singletonMu.Lock() - defer singletonMu.Unlock() + singletonMu.Lock() if rdu2HostsSingleton == nil { rdu2HostsSingletonTmp, err := newRDU2Hosts() if err != nil { + singletonMu.Unlock() return nil, err } rdu2HostsSingleton = rdu2HostsSingletonTmp } + singleton := rdu2HostsSingleton + singletonMu.Unlock() // Create a copy of the map to prevent external modifications - rdu2HostsSingleton.mu.RLock() - defer rdu2HostsSingleton.mu.RUnlock() + singleton.mu.RLock() + defer singleton.mu.RUnlock() - hostsCopy := make(map[string]*RDU2Host, len(rdu2HostsSingleton.hostsMap)) - for k, v := range rdu2HostsSingleton.hostsMap { + hostsCopy := make(map[string]*RDU2Host, len(singleton.hostsMap)) + for k, v := range singleton.hostsMap {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 85 - 99, GetRDU2HostsList currently holds singletonMu across the entire map-copy operation which serializes all callers; change it to lock singletonMu only to check/create rdu2HostsSingleton (call singletonMu.Lock(), if nil create and assign, then singletonMu.Unlock()), then separately acquire rdu2HostsSingleton.mu.RLock() to copy the map and defer that RUnlock; reference singletonMu, rdu2HostsSingleton, GetRDU2HostsList and rdu2HostsSingleton.mu to locate the changes and ensure no defer keeps singletonMu locked during the copy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 50-55: The code currently builds hostsFilePath from
os.Getenv("SHARED_DIR") which may be empty; update the function to explicitly
check the SHARED_DIR env var (via os.Getenv or os.LookupEnv) and return an error
immediately if it's unset or empty instead of falling back to the working
directory, then construct hostsFilePath with path.Join(sharedDir, "hosts.yaml")
and proceed to read it with os.ReadFile; reference the existing hostsFilePath
variable, the os.Getenv("SHARED_DIR") call, and the subsequent os.ReadFile usage
so you fail fast when SHARED_DIR is not provided.
---
Nitpick comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 85-99: GetRDU2HostsList currently holds singletonMu across the
entire map-copy operation which serializes all callers; change it to lock
singletonMu only to check/create rdu2HostsSingleton (call singletonMu.Lock(), if
nil create and assign, then singletonMu.Unlock()), then separately acquire
rdu2HostsSingleton.mu.RLock() to copy the map and defer that RUnlock; reference
singletonMu, rdu2HostsSingleton, GetRDU2HostsList and rdu2HostsSingleton.mu to
locate the changes and ensure no defer keeps singletonMu locked during the copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bff2e654-0eda-40f7-bbba-61ab00de33d3
📒 Files selected for processing (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go (1)
89-105: Reduce lock contention inGetRDU2HostsListby shorteningsingletonMuscope.Lines 89-105 keep
singletonMuheld during the full copy flow, serializing concurrent readers unnecessarily. Lock only for initialization/pointer publication, then copy under the per-instance read lock.Proposed refactor
func GetRDU2HostsList() (map[string]*RDU2Host, error) { singletonMu.Lock() - defer singletonMu.Unlock() - if rdu2HostsSingleton == nil { rdu2HostsSingletonTmp, err := newRDU2Hosts() if err != nil { + singletonMu.Unlock() return nil, err } rdu2HostsSingleton = rdu2HostsSingletonTmp } + singleton := rdu2HostsSingleton + singletonMu.Unlock() // Create a copy of the map to prevent external modifications - rdu2HostsSingleton.mu.RLock() - defer rdu2HostsSingleton.mu.RUnlock() + singleton.mu.RLock() + defer singleton.mu.RUnlock() - hostsCopy := make(map[string]*RDU2Host, len(rdu2HostsSingleton.hostsMap)) - for k, v := range rdu2HostsSingleton.hostsMap { + hostsCopy := make(map[string]*RDU2Host, len(singleton.hostsMap)) + for k, v := range singleton.hostsMap {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 89 - 105, GetRDU2HostsList is holding the package-level singletonMu for the entire copy operation which serializes readers; change the flow so singletonMu is only held to check/initialize/publish rdu2HostsSingleton (use singletonMu.Lock/unlock around the newRDU2Hosts call and assignment), then release singletonMu and acquire the instance-level rdu2HostsSingleton.mu.RLock to perform the hostsMap copy into hostsCopy (iterate over rdu2HostsSingleton.hostsMap and copy into a new map of type map[string]*RDU2Host), ensuring newRDU2Hosts is called only under singletonMu and the per-instance read lock protects map traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 63-66: The YAML parsing currently uses yaml.Unmarshal(yamlBytes,
&hostsData) which ignores unknown keys; change this to a strict parse so unknown
fields fail the load (either replace with yaml.UnmarshalStrict(yamlBytes,
&hostsData) or create a yaml.NewDecoder(bytes.NewReader(yamlBytes)) and call
d.KnownFields(true).Decode(&hostsData)) so typos in hosts.yaml surface as
errors; keep the same error wrapping logic around the parse failure for
hostsData.
---
Nitpick comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 89-105: GetRDU2HostsList is holding the package-level singletonMu
for the entire copy operation which serializes readers; change the flow so
singletonMu is only held to check/initialize/publish rdu2HostsSingleton (use
singletonMu.Lock/unlock around the newRDU2Hosts call and assignment), then
release singletonMu and acquire the instance-level rdu2HostsSingleton.mu.RLock
to perform the hostsMap copy into hostsCopy (iterate over
rdu2HostsSingleton.hostsMap and copy into a new map of type
map[string]*RDU2Host), ensuring newRDU2Hosts is called only under singletonMu
and the per-instance read lock protects map traversal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aca99c7c-546e-4128-bf15-31a734582322
📒 Files selected for processing (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go (1)
3-16:⚠️ Potential issue | 🔴 CriticalMissing
bytesimport causes compilation failure.Line 63 uses
bytes.NewReader(yamlBytes)but thebytespackage is not imported. This will cause a compilation error.🔧 Proposed fix
import ( + "bytes" "context" "fmt" "os"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 3 - 16, The file is missing the bytes package import used by bytes.NewReader(yamlBytes); update the import block to include "bytes" so the call to bytes.NewReader in the code (search for bytes.NewReader(yamlBytes)) compiles successfully—add "bytes" alongside the existing imports in the top-level import list.
🧹 Nitpick comments (2)
test/extended/util/compat_otp/upibaremetalrdu_client.go (2)
29-32: Consider usingsync.Oncefor singleton initialization.The codebase's
test_env.gousessync.Oncefor the same pattern. The current approach holdssingletonMufor the entireGetRDU2HostsListcall, blocking concurrent reads even after initialization.♻️ Proposed refactor using sync.Once
var ( rdu2HostsSingleton *RDU2Hosts - singletonMu sync.Mutex + singletonOnce sync.Once + singletonErr error )Then in
GetRDU2HostsList:func GetRDU2HostsList() (map[string]*RDU2Host, error) { singletonOnce.Do(func() { rdu2HostsSingleton, singletonErr = newRDU2Hosts() }) if singletonErr != nil { return nil, singletonErr } rdu2HostsSingleton.mu.RLock() defer rdu2HostsSingleton.mu.RUnlock() // ... deep copy logic }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 29 - 32, Replace the current mutex-based lazy init with sync.Once: add a package-level singletonOnce sync.Once and singletonErr error, then change GetRDU2HostsList to call singletonOnce.Do(func(){ rdu2HostsSingleton, singletonErr = newRDU2Hosts() }); after Do check singletonErr and return it if set; only then take rdu2HostsSingleton.mu.RLock()/RUnlock() and perform the deep-copy logic. Remove holding singletonMu for the whole function and drop singletonMu usage so concurrent readers aren't blocked after initialization.
70-80: Consider validating that hosts.yaml contains at least one host.If
hosts.yamlis empty or contains no valid entries, this returns an empty map without error. Depending on your test requirements, this might indicate a misconfiguration worth failing early on.💡 Optional validation
hostsMap := make(map[string]*RDU2Host, len(hostsData)) + if len(hostsData) == 0 { + return nil, fmt.Errorf("hosts.yaml contains no host entries") + } for i := range hostsData {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 70 - 80, Add a guard to fail fast when the input slice hostsData (or the resulting hostsMap) contains no hosts: in the code block that builds hostsMap (the loop over hostsData and creation of hostsMap := make(map[string]*RDU2Host, len(hostsData))), check if len(hostsData) == 0 (or check len(hostsMap) after the loop) and return a descriptive error (e.g., fmt.Errorf("hosts.yaml contains no hosts")) instead of returning an empty map; update the function return path where hostsMap is returned so callers receive an error for an empty hosts file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 3-16: The file is missing the bytes package import used by
bytes.NewReader(yamlBytes); update the import block to include "bytes" so the
call to bytes.NewReader in the code (search for bytes.NewReader(yamlBytes))
compiles successfully—add "bytes" alongside the existing imports in the
top-level import list.
---
Nitpick comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Around line 29-32: Replace the current mutex-based lazy init with sync.Once:
add a package-level singletonOnce sync.Once and singletonErr error, then change
GetRDU2HostsList to call singletonOnce.Do(func(){ rdu2HostsSingleton,
singletonErr = newRDU2Hosts() }); after Do check singletonErr and return it if
set; only then take rdu2HostsSingleton.mu.RLock()/RUnlock() and perform the
deep-copy logic. Remove holding singletonMu for the whole function and drop
singletonMu usage so concurrent readers aren't blocked after initialization.
- Around line 70-80: Add a guard to fail fast when the input slice hostsData (or
the resulting hostsMap) contains no hosts: in the code block that builds
hostsMap (the loop over hostsData and creation of hostsMap :=
make(map[string]*RDU2Host, len(hostsData))), check if len(hostsData) == 0 (or
check len(hostsMap) after the loop) and return a descriptive error (e.g.,
fmt.Errorf("hosts.yaml contains no hosts")) instead of returning an empty map;
update the function return path where hostsMap is returned so callers receive an
error for an empty hosts file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aec9e2fa-9356-4611-9e33-214946988eb5
📒 Files selected for processing (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/extended/util/compat_otp/upibaremetalrdu_client.go (3)
55-55: Usefilepath.Joininstead ofpath.Joinfor filesystem paths.The
pathpackage is designed for URL-style forward-slash paths, whilefilepath.Joinhandles OS-specific path separators correctly. On Windows,path.Joinwould produce paths with forward slashes that may not work with filesystem operations.♻️ Proposed fix
Update the import:
- "path" + "path/filepath"Update the usage:
- hostsFilePath := path.Join(sharedDir, "hosts.yaml") + hostsFilePath := filepath.Join(sharedDir, "hosts.yaml")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` at line 55, Replace path.Join with filepath.Join for filesystem paths to ensure OS-specific separators: in upibaremetalrdu_client.go update the import to include "path/filepath" (remove or stop using the "path" import) and change the hostsFilePath assignment (and any other uses) from path.Join(sharedDir, "hosts.yaml") to filepath.Join(sharedDir, "hosts.yaml") so file paths are constructed correctly across platforms.
91-118: LGTM on the singleton accessor with deep copy.The implementation correctly:
- Uses mutex for thread-safe lazy initialization
- Performs a true deep copy of host values to prevent external mutation
- Handles nil values defensively
Optional optimization: The exclusive
singletonMulock is held for the entire operation, serializing all calls even after initialization. Consider usingsync.Oncefor initialization or releasingsingletonMubefore acquiring the read lock to allow concurrent reads post-initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 91 - 118, GetRDU2HostsList currently holds singletonMu for the entire function which serializes all callers; change initialization to use sync.Once (e.g., a package-level once var and newRDU2Hosts in the Once.Do) or at minimum release singletonMu immediately after ensuring rdu2HostsSingleton is non-nil and before acquiring rdu2HostsSingleton.mu.RLock(); keep the existing deep-copy logic and ensure you still guard access to rdu2HostsSingleton.hostsMap with rdu2HostsSingleton.mu while initializing only uses singletonMu (or sync.Once) and references rdu2HostsSingleton, singletonMu, newRDU2Hosts and rdu2HostsSingleton.mu accordingly.
44-47: LGTM on the new fields.The additional host metadata fields are well-structured with appropriate YAML tags.
Nitpick: Go convention typically uses all-caps for acronyms—
RedfishBaseURIinstead ofRedfishBaseUri. This is a minor style consistency point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/util/compat_otp/upibaremetalrdu_client.go` around lines 44 - 47, Rename the struct field RedfishBaseUri to RedfishBaseURI to follow Go acronym casing conventions; keep the yaml tag `redfish_base_uri` unchanged. Update all usages/references of the field (e.g., assignments, struct literals, tests, unmarshalling helpers) to the new identifier RedfishBaseURI so the code compiles and the YAML mapping remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/util/compat_otp/upibaremetalrdu_client.go`:
- Line 55: Replace path.Join with filepath.Join for filesystem paths to ensure
OS-specific separators: in upibaremetalrdu_client.go update the import to
include "path/filepath" (remove or stop using the "path" import) and change the
hostsFilePath assignment (and any other uses) from path.Join(sharedDir,
"hosts.yaml") to filepath.Join(sharedDir, "hosts.yaml") so file paths are
constructed correctly across platforms.
- Around line 91-118: GetRDU2HostsList currently holds singletonMu for the
entire function which serializes all callers; change initialization to use
sync.Once (e.g., a package-level once var and newRDU2Hosts in the Once.Do) or at
minimum release singletonMu immediately after ensuring rdu2HostsSingleton is
non-nil and before acquiring rdu2HostsSingleton.mu.RLock(); keep the existing
deep-copy logic and ensure you still guard access to rdu2HostsSingleton.hostsMap
with rdu2HostsSingleton.mu while initializing only uses singletonMu (or
sync.Once) and references rdu2HostsSingleton, singletonMu, newRDU2Hosts and
rdu2HostsSingleton.mu accordingly.
- Around line 44-47: Rename the struct field RedfishBaseUri to RedfishBaseURI to
follow Go acronym casing conventions; keep the yaml tag `redfish_base_uri`
unchanged. Update all usages/references of the field (e.g., assignments, struct
literals, tests, unmarshalling helpers) to the new identifier RedfishBaseURI so
the code compiles and the YAML mapping remains the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 061376d9-d13c-4302-a4ab-251796bd302c
📒 Files selected for processing (1)
test/extended/util/compat_otp/upibaremetalrdu_client.go
|
Scheduling required tests: Scheduling tests matching the |
|
Quick question on the Is that the intended behavior? If this only runs in CI where If retry-on-failure is needed, you could replace var (
rdu2HostsSingleton *RDU2Hosts
singletonMu sync.Mutex
)
func GetRDU2HostsList() (map[string]*RDU2Host, error) {
singletonMu.Lock()
defer singletonMu.Unlock()
if rdu2HostsSingleton != nil {
return rdu2HostsSingleton.copyMap(), nil
}
s, err := newRDU2Hosts()
if err != nil {
return nil, err // next call will retry
}
rdu2HostsSingleton = s
return s.copyMap(), nil
}This way, if initialization fails, the next call will attempt to load again rather than returning the cached error. |
|
Scheduling required tests: Scheduling tests matching the |
|
Job Failure Risk Analysis for sha: 3cb7c99
|
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@rioliu-rh please review |
|
@coderabbitai are all the review comments are resolved, are we good to merge |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 47414
All major CodeRabbit comments are resolved ✅
Your One nitpick still open (non-blocking) func GetRDU2HostsList() (map[string]*RDU2Host, error) {
singletonMu.Lock()
if rdu2HostsSingleton == nil {
s, err := newRDU2Hosts()
if err != nil {
singletonMu.Unlock()
return nil, err
}
rdu2HostsSingleton = s
}
singleton := rdu2HostsSingleton
singletonMu.Unlock()
return singleton.copyMap(), nil
}Given this is test infrastructure code (not a high-concurrency hot path), this is low priority and can be deferred. The PR is otherwise clean and all actionable concerns have been addressed. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rioliu-rh, sgoveas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by sgoveas |
|
@sgoveas: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Scheduling required tests: Scheduling tests matching the |
|
/test e2e-metal-ovn-two-node-fencing |
|
/retest |
|
/test e2e-aws-ovn-microshift |
|
@sgoveas: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary by CodeRabbit