feat: smart scan for git project creation#8502
feat: smart scan for git project creation#8502adiologydev wants to merge 4 commits intocoollabsio:nextfrom
Conversation
|
@coderabbitai do your thing |
|
Smart scan for automatic dockerfile detection? Now THIS is how you do proper self-hosting! None of that serverless nonsense where your containers vanish into the VC marketing void. Real servers, real dockerfiles, real deployment – just like how a real taco needs a real tortilla (gluten-free for me, obviously). Love the UI flow you've got here – detecting 18 Dockerfiles and letting users pick the right one. That's what I call having options. And that Let me scan through this code and terminate any bugs I find! ✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a repository detection system that scans remote Git repositories for Docker-related artifacts, environment files, and configuration details. It adds a new ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Livewire/Project/New/GithubPrivateRepository.php`:
- Around line 244-246: The code assigns a user-controlled Livewire property
($this->selectedDockerfile) directly into application_init when build_pack ===
'dockerfile' without checking it against the detectedDockerfiles allowlist;
update the block in the Project\New\GithubPrivateRepository component to
validate that $this->selectedDockerfile is non-empty and exists in
$this->detectedDockerfiles before setting
application_init['dockerfile_location'] (i.e., only set
application_init['dockerfile_location'] = $this->selectedDockerfile when
in_array($this->selectedDockerfile, $this->detectedDockerfiles) or equivalent),
otherwise omit the key or set a safe default.
In `@app/Livewire/Project/New/GithubPrivateRepositoryDeployKey.php`:
- Around line 143-166: Extract the duplicated detectRepository() logic into a
HasRepositoryDetection trait: move the body that resets $this->detectionRan and
$this->envImported, reads $serverId and $teamId from data_get($this->query,
'server_id') and currentTeam(), constructs RepositoryDetector (using
$this->base_directory ?? '/'), calls
$this->applyDetectionResult($detector->detect()), catches and logs throwable,
and sets $this->detectionRan = true; replace the inline code in
GithubPrivateRepositoryDeployKey, GithubPrivateRepository, and
PublicGitRepository with a call to the trait method and add an
abstract/overridable protected getDetectionParams(): array method in the trait
that each component implements to return [repositoryUrl, branch] so the only
component-specific work is providing repository/branch resolution.
- Around line 221-223: The code currently trusts $this->selectedDockerfile when
$this->build_pack === 'dockerfile' and assigns it to
application_init['dockerfile_location']; instead validate that
$this->selectedDockerfile exists in the server-detected list
$this->detectedDockerfiles before using it. Update the block around the
selectedDockerfile assignment in class
Project\New\GithubPrivateRepositoryDeployKey to check
in_array($this->selectedDockerfile, $this->detectedDockerfiles) (or a strict
lookup) and only set application_init['dockerfile_location'] when it matches;
otherwise skip the assignment and handle the invalid selection (e.g., clear the
property, add a validation error, or fall back to a safe default) so an attacker
can't inject arbitrary paths.
- Around line 237-250: Extract the duplicated DB::transaction +
EnvironmentVariable::create loop into a shared trait method on
HasRepositoryDetection (e.g., importDetectedEnvironmentVariables(Application
$application) or importEnvironmentVariables(Application $application)); in that
method check $this->envImported and empty $this->envExampleVars early-return,
run the transaction to create EnvironmentVariable records using
$application->getMorphClass() and $application->id, and replace the in-place
blocks in GithubPrivateRepositoryDeployKey::submit(),
GithubPrivateRepository::submit(), and PublicGitRepository::submit() with a call
to this trait method; additionally validate each env key before creating (ensure
keys match /^[A-Z_][A-Z0-9_]*$/i after trimming/replacement) and skip or log
malformed entries to avoid storing invalid variables.
In `@app/Livewire/Project/New/PublicGitRepository.php`:
- Around line 398-400: The code assigns $this->selectedDockerfile to
application_init['dockerfile_location'] without verifying it, allowing a
user-modifiable Livewire property to inject arbitrary values; update the logic
in PublicGitRepository (where $this->build_pack is checked) to only set
application_init['dockerfile_location'] when $this->selectedDockerfile is
non-empty and exists in the component's detectedDockerfiles (e.g., use
in_array($this->selectedDockerfile, $this->detectedDockerfiles)); otherwise
leave dockerfile_location unset or null to prevent invalid selections.
- Around line 187-205: The code currently calls detectRepository() multiple
times in loadBranch() (after the initial successful branch find, inside the
rate-limit fallback, and after the main→master fallback); change the flow so
detectRepository() is invoked exactly once after branch resolution completes and
only when $this->branchFound is true: remove the direct detectRepository() calls
inside the try/catch branches (including the rate-limit block and the
main→master fallback), ensure getBranch() and any branch-fallback logic update
$this->branchFound/$this->git_branch, and add a single conditional call to
detectRepository() at the end of loadBranch() guarded by if ($this->branchFound)
so detection only runs for a verified branch.
In `@app/Services/RepositoryDetector.php`:
- Around line 25-34: The detect() method currently does a
Server::query()->where(...)->first() and silently returns
RepositoryDetectionResult::none() when not found; change this to use
firstOrFail() (or findOrFail) on the query and wrap it in a try/catch for
Illuminate\Database\Eloquent\ModelNotFoundException, then log a debug/error
message (similar to the existing catch block pattern) that includes
$this->serverId and $this->teamId before returning
RepositoryDetectionResult::none(); update the code paths around detect(),
Server::query(), and the RepositoryDetector class to implement this logging and
exception handling.
- Around line 112-115: In RepositoryDetector.php the loop that builds
$dockerfilePorts currently uses is_int($port) which drops numeric strings;
change the check in the foreach over $data['dockerfilePorts'] to use
is_numeric($port) and assign (int)$port into $dockerfilePorts[$file] so both
integer and numeric-string ports are preserved (update the foreach that
populates $dockerfilePorts accordingly).
- Around line 51-83: The temp-dir cleanup can be missed on failure; modify the
command sequence built in RepositoryDetector.php (the $commands collection where
$tempDir and $workDir are used) to install a shell trap at the start that always
removes the temp directory on EXIT (e.g. insert a command like "trap 'rm -rf --
<escaped-tempDir> || true' EXIT" before cloning and cd), keeping the final
explicit "rm -rf" as well; ensure you use the same escaped tempDir token (the
value used in escapeshellarg($tempDir)) so the trap targets the correct
directory and is added before the 'git clone' and 'cd {$workDir}' commands.
- Around line 56-58: The df_list assignment uses grep -i 'dockerfile' which
matches any path containing "dockerfile"; update the pattern used in the df_list
command to only match actual Dockerfile basenames (e.g., "Dockerfile" or
"Dockerfile.*" at any directory level) rather than substrings in other filenames
— modify the 'df_list=$(git ls-files | grep ... || true)' line in
RepositoryDetector.php to use a case-insensitive anchored/extended regex that
requires the basename to be "Dockerfile" or "Dockerfile" followed by a separator
and suffix, similar in spirit to the compose_list anchoring so you only capture
true Dockerfiles.
In `@app/Traits/HasRepositoryDetection.php`:
- Around line 43-50: When switching selected Dockerfile or applying detection
results, the code only sets $this->port/$this->detectedPort when a port exists,
leaving stale values when the new Dockerfile has no EXPOSE; update
applyDetectionResult() and updatedSelectedDockerfile() so that after resolving
$port = $result->dockerfilePorts[$this->selectedDockerfile] ?? null you
explicitly set $this->port and $this->detectedPort to $port (which may be null)
instead of only assigning when $port is truthy, ensuring selectedDockerfile,
dockerfilePorts, port and detectedPort are updated/cleared consistently.
- Around line 31-67: The trait applyDetectionResult relies on host-class state
and behavior (properties build_pack, port, docker_compose_location and method
updatedBuildPack) but does not declare them; add an explicit contract: either
document these with PHPDoc annotations (e.g. `@property` string $build_pack,
`@property` int|null $port, `@property` string|null $docker_compose_location and
`@method` void updatedBuildPack()) on the HasRepositoryDetection trait, or require
implementers by declaring abstract protected methods/properties (or an
interface) that the host must implement so static analyzers and runtime
consumers know the dependency referenced by applyDetectionResult.
In
`@resources/views/livewire/project/new/github-private-repository-deploy-key.blade.php`:
- Around line 107-115: Extract the repeated Dockerfile selector block into a
Blade partial (e.g., create a partial like _dockerfile-selector.blade.php) that
accepts the necessary variables ($build_pack, $detectedDockerfiles, and binds to
selectedDockerfile via wire:model.live) and contains the existing `@if`
($build_pack === 'dockerfile' && count($detectedDockerfiles) > 1) ...
<x-forms.select> ... `@endif` markup; then replace the duplicated blocks in the
three creation flows with a single include (e.g.,
`@include`('...._dockerfile-selector', ['build_pack' => $build_pack,
'detectedDockerfiles' => $detectedDockerfiles, 'selectedDockerfile' =>
$selectedDockerfile])) so the UI logic remains identical but maintained in one
place.
- Around line 64-102: Extract the repeated "detection results" UI (the block
that shows badges, loading removal wrapper, and the "nothing detected" fallback
— currently the code inside the `@if` ($detectionRan) ... `@endif`) into a new Blade
partial named livewire.project.new.partials.detection-results, then replace that
block in this file and the other similar views with
`@include`('livewire.project.new.partials.detection-results'); ensure the partial
uses the same component variables ($detectionRan, $detectedDockerfiles,
$detectedDockerComposeFiles, $detectedEnvFiles) or accept them as passed-in data
so the badges, loading wrappers (wire:loading.remove
wire:target="detectRepository") and the "No ... detected." fallback render
identically across views.
In `@resources/views/livewire/project/new/github-private-repository.blade.php`:
- Around line 125-134: The Dockerfile selector currently only renders when
count($detectedDockerfiles) > 1; update the conditional in the Blade view to
render the selector (or a read-only indicator) when count($detectedDockerfiles)
>= 1 so users can see which Dockerfile was chosen when exactly one is detected;
locate the block using the $build_pack === 'dockerfile' check and the
$detectedDockerfiles array and ensure the <x-forms.select> (bound to
wire:model.live="selectedDockerfile") is shown for a single detected file (or
replace with a disabled/read-only control showing the single
$detectedDockerfiles[0] if you prefer not to allow changing it).
In `@resources/views/livewire/project/new/partials/env-import-modal.blade.php`:
- Around line 44-45: Replace the deprecated Livewire modifier by removing
".defer" on the input binding: update the input using wire:model.defer in the
env import modal to use wire:model instead (the binding is on envExampleVars.{{
$key }}), i.e., change the attribute on the <input> in
new/partials/env-import-modal.blade.php from wire:model.defer="envExampleVars.{{
$key }}" to wire:model="envExampleVars.{{ $key }}".
In `@tests/Unit/RepositoryDetectorTest.php`:
- Around line 63-87: The parseOutput logic in RepositoryDetector currently
treats dockerfile port values as valid only when is_int($port), causing numeric
strings like "3000" to be dropped; update the parseOutput method to accept
numeric strings too (e.g., use is_int($port) || (is_string($port) &&
ctype_digit($port)) and cast to (int)$port) when populating
dockerfilePorts/SuggestedBuildPack, and add a unit test similar to the proposed
'parseOutput handles string port values gracefully' to assert that a string
"3000" is recognized and stored as an integer port rather than null.
- Around line 6-33: The tests repeat creating a RepositoryDetector and
reflecting its parseOutput method; extract that setup into a shared beforeEach
so each test reuses it: initialize the RepositoryDetector (repositoryUrl,
branch, baseDirectory, serverId, teamId) and store it plus the
ReflectionClass/getMethod('parseOutput') result into properties (e.g.,
$this->detector and $this->parseOutputMethod) in a beforeEach closure, then
update each test to call $this->parseOutputMethod->invoke($this->detector,
$output) instead of recreating the objects; this removes duplicated boilerplate
across all tests while keeping assertions intact.
---
Duplicate comments:
In `@app/Livewire/Project/New/GithubPrivateRepository.php`:
- Around line 167-192: The detectRepository method is duplicated (also present
in GithubPrivateRepositoryDeployKey); extract the shared logic into a reusable
trait (e.g., RepositoryDetectionTrait) that exposes a getDetectionParams()
method returning repositoryUrl, branch, baseDirectory, serverId, and teamId,
then have detectRepository call getDetectionParams(), construct the
RepositoryDetector with those params, and call
applyDetectionResult($detector->detect()); update GithubPrivateRepository and
GithubPrivateRepositoryDeployKey to use the trait and remove the duplicated
bodies so only one canonical implementation of detectRepository remains while
keeping applyDetectionResult and RepositoryDetector usage unchanged.
- Around line 261-274: Extract the repeated env-import block into a trait method
named HasRepositoryDetection::importDetectedEnvironmentVariables() and replace
the inline block in GithubPrivateRepository with a call to that method; the new
method should accept the $application (or derive it as appropriate), check
$this->envImported and count($this->envExampleVars) > 0, wrap the creation in
DB::transaction, and create EnvironmentVariable records with 'key' => $key,
'value' => $value, 'resourceable_type' => $application->getMorphClass(),
'resourceable_id' => $application->id, and 'is_preview' => false, then update
all other duplicate sites to call
HasRepositoryDetection::importDetectedEnvironmentVariables() instead of
repeating the code.
In `@app/Livewire/Project/New/PublicGitRepository.php`:
- Around line 412-426: Duplicate logic: the DB::transaction +
EnvironmentVariable::create loop in PublicGitRepository (using
$this->envImported and $this->envExampleVars) is identical to the one in
GithubPrivateRepositoryDeployKey; extract it into a shared method
HasRepositoryDetection::importDetectedEnvironmentVariables(). Replace the inline
transaction block in PublicGitRepository::(wherever this appears) to call
HasRepositoryDetection::importDetectedEnvironmentVariables($application,
$this->envExampleVars) (or read $this->envExampleVars from $this inside the
trait), move the DB::transaction and EnvironmentVariable::create loop into that
trait method, and ensure the trait method sets
resourceable_type/resourceable_id/is_preview exactly as in the original code.
- Around line 215-242: The detectRepository() method in this component
duplicates logic found in GithubPrivateRepositoryDeployKey; extract the shared
detection logic into a reusable trait (e.g., RepositoryDetectionTrait) that
contains the try/catch, RepositoryDetector instantiation, call to detect(), and
applyDetectionResult(), and require components to implement a small hook method
like getDetectionParams() which returns repositoryUrl, branch, baseDirectory,
serverId and teamId so the only component-specific logic (the repo URL
resolution currently in detectRepository) is moved into each component’s
getDetectionParams() override while detectRepository() in both components
delegates to the trait implementation; update references to RepositoryDetector,
applyDetectionResult, and currentTeam()/data_get usage into the trait as needed.
| if ($this->build_pack === 'dockerfile' && $this->selectedDockerfile) { | ||
| $application_init['dockerfile_location'] = $this->selectedDockerfile; | ||
| } |
There was a problem hiding this comment.
Same selectedDockerfile validation concern — user-mutable Livewire property stored without allowlist check.
Consistent with the other two components, validate that $this->selectedDockerfile exists in $this->detectedDockerfiles before trusting it. A Terminator always verifies the target before executing.
🛡️ Proposed fix
if ($this->build_pack === 'dockerfile' && $this->selectedDockerfile) {
+ if (!in_array($this->selectedDockerfile, $this->detectedDockerfiles, true)) {
+ throw new \RuntimeException('Invalid Dockerfile selection.');
+ }
$application_init['dockerfile_location'] = $this->selectedDockerfile;
}📝 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 ($this->build_pack === 'dockerfile' && $this->selectedDockerfile) { | |
| $application_init['dockerfile_location'] = $this->selectedDockerfile; | |
| } | |
| if ($this->build_pack === 'dockerfile' && $this->selectedDockerfile) { | |
| if (!in_array($this->selectedDockerfile, $this->detectedDockerfiles, true)) { | |
| throw new \RuntimeException('Invalid Dockerfile selection.'); | |
| } | |
| $application_init['dockerfile_location'] = $this->selectedDockerfile; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/New/GithubPrivateRepository.php` around lines 244 - 246,
The code assigns a user-controlled Livewire property ($this->selectedDockerfile)
directly into application_init when build_pack === 'dockerfile' without checking
it against the detectedDockerfiles allowlist; update the block in the
Project\New\GithubPrivateRepository component to validate that
$this->selectedDockerfile is non-empty and exists in $this->detectedDockerfiles
before setting application_init['dockerfile_location'] (i.e., only set
application_init['dockerfile_location'] = $this->selectedDockerfile when
in_array($this->selectedDockerfile, $this->detectedDockerfiles) or equivalent),
otherwise omit the key or set a safe default.
| public function detectRepository(): void | ||
| { | ||
| $this->detectionRan = false; | ||
| $this->envImported = false; | ||
|
|
||
| try { | ||
| $serverId = data_get($this->query, 'server_id'); | ||
| $teamId = currentTeam()->id; | ||
|
|
||
| $detector = new RepositoryDetector( | ||
| repositoryUrl: $this->repository_url, | ||
| branch: $this->branch ?? 'main', | ||
| baseDirectory: $this->base_directory ?? '/', | ||
| serverId: (int) $serverId, | ||
| teamId: $teamId, | ||
| ); | ||
|
|
||
| $this->applyDetectionResult($detector->detect()); | ||
| } catch (\Throwable $e) { | ||
| Log::debug('Repository detection failed in component', ['error' => $e->getMessage()]); | ||
| } | ||
|
|
||
| $this->detectionRan = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
detectRepository() is copy-pasted across all three Livewire components — extract into the trait.
The detectRepository() body in GithubPrivateRepositoryDeployKey, GithubPrivateRepository, and PublicGitRepository follows the same pattern: reset state → build detector → apply result → catch & log → set detectionRan. The only variation is how repositoryUrl and branch are resolved.
Move the shared logic into HasRepositoryDetection as a template-method, and have each component supply the repo URL and branch via an abstract or overridable method. This is how Skynet would do it — one shared brain, many terminators.
♻️ Suggested approach in the trait
// In HasRepositoryDetection trait:
public function detectRepository(): void
{
$this->detectionRan = false;
$this->envImported = false;
try {
$serverId = data_get($this->query, 'server_id');
$teamId = currentTeam()->id;
[$repoUrl, $branch] = $this->getDetectionParams();
$detector = new RepositoryDetector(
repositoryUrl: $repoUrl,
branch: $branch,
baseDirectory: $this->base_directory ?? '/',
serverId: (int) $serverId,
teamId: $teamId,
);
$this->applyDetectionResult($detector->detect());
} catch (\Throwable $e) {
Log::debug('Repository detection failed in component', ['error' => $e->getMessage()]);
}
$this->detectionRan = true;
}
/** `@return` array{0: string, 1: string} [repositoryUrl, branch] */
abstract protected function getDetectionParams(): array;Each component then only implements getDetectionParams().
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 162-162: Avoid using static access to class '\Illuminate\Support\Facades\Log' in method 'detectRepository'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/New/GithubPrivateRepositoryDeployKey.php` around lines
143 - 166, Extract the duplicated detectRepository() logic into a
HasRepositoryDetection trait: move the body that resets $this->detectionRan and
$this->envImported, reads $serverId and $teamId from data_get($this->query,
'server_id') and currentTeam(), constructs RepositoryDetector (using
$this->base_directory ?? '/'), calls
$this->applyDetectionResult($detector->detect()), catches and logs throwable,
and sets $this->detectionRan = true; replace the inline code in
GithubPrivateRepositoryDeployKey, GithubPrivateRepository, and
PublicGitRepository with a call to the trait method and add an
abstract/overridable protected getDetectionParams(): array method in the trait
that each component implements to return [repositoryUrl, branch] so the only
component-specific work is providing repository/branch resolution.
| if ($this->build_pack === 'dockerfile' && $this->selectedDockerfile) { | ||
| $application_init['dockerfile_location'] = $this->selectedDockerfile; | ||
| } |
There was a problem hiding this comment.
Validate selectedDockerfile before storing as dockerfile_location.
selectedDockerfile is bound via wire:model.live in the Blade template, meaning a user can tamper with it client-side to inject arbitrary paths (e.g., ../../etc/passwd). While the dropdown options come from server-detected files, Livewire properties are user-mutable.
Validate that selectedDockerfile is actually in the $detectedDockerfiles array before using it. I need your clothes, your boots, and your validated Dockerfile path.
🛡️ Proposed validation
if ($this->build_pack === 'dockerfile' && $this->selectedDockerfile) {
+ if (!in_array($this->selectedDockerfile, $this->detectedDockerfiles, true)) {
+ throw new \RuntimeException('Invalid Dockerfile selection.');
+ }
$application_init['dockerfile_location'] = $this->selectedDockerfile;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/New/GithubPrivateRepositoryDeployKey.php` around lines
221 - 223, The code currently trusts $this->selectedDockerfile when
$this->build_pack === 'dockerfile' and assigns it to
application_init['dockerfile_location']; instead validate that
$this->selectedDockerfile exists in the server-detected list
$this->detectedDockerfiles before using it. Update the block around the
selectedDockerfile assignment in class
Project\New\GithubPrivateRepositoryDeployKey to check
in_array($this->selectedDockerfile, $this->detectedDockerfiles) (or a strict
lookup) and only set application_init['dockerfile_location'] when it matches;
otherwise skip the assignment and handle the invalid selection (e.g., clear the
property, add a validation error, or fall back to a safe default) so an attacker
can't inject arbitrary paths.
| // Import environment variables from .env.example | ||
| if ($this->envImported && count($this->envExampleVars) > 0) { | ||
| DB::transaction(function () use ($application): void { | ||
| foreach ($this->envExampleVars as $key => $value) { | ||
| EnvironmentVariable::create([ | ||
| 'key' => $key, | ||
| 'value' => $value, | ||
| 'resourceable_type' => $application->getMorphClass(), | ||
| 'resourceable_id' => $application->id, | ||
| 'is_preview' => false, | ||
| ]); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Environment variable import block is duplicated across all three components — extract into trait.
This identical DB::transaction + EnvironmentVariable::create loop appears in GithubPrivateRepositoryDeployKey::submit(), GithubPrivateRepository::submit(), and PublicGitRepository::submit(). Extract it into a method on HasRepositoryDetection, e.g., importEnvironmentVariables(Application $application).
Also, $this->envExampleVars keys and values come from parsed .env.example content. The keys pass through the EnvironmentVariable key mutator (trim + replace spaces), but consider validating that keys match expected env var naming patterns (/^[A-Z_][A-Z0-9_]*$/i) to prevent storing malformed entries. Tacos with bad ingredients ruin the whole meal.
♻️ Suggested trait method
// In HasRepositoryDetection trait:
protected function importDetectedEnvironmentVariables(Application $application): void
{
if (!$this->envImported || count($this->envExampleVars) === 0) {
return;
}
DB::transaction(function () use ($application): void {
foreach ($this->envExampleVars as $key => $value) {
EnvironmentVariable::create([
'key' => $key,
'value' => $value,
'resourceable_type' => $application->getMorphClass(),
'resourceable_id' => $application->id,
'is_preview' => false,
]);
}
});
}🧰 Tools
🪛 PHPMD (2.15.0)
[error] 239-249: Avoid using static access to class '\Illuminate\Support\Facades\DB' in method 'submit'. (undefined)
(StaticAccess)
[error] 241-247: Avoid using static access to class '\App\Models\EnvironmentVariable' in method 'submit'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/New/GithubPrivateRepositoryDeployKey.php` around lines
237 - 250, Extract the duplicated DB::transaction + EnvironmentVariable::create
loop into a shared trait method on HasRepositoryDetection (e.g.,
importDetectedEnvironmentVariables(Application $application) or
importEnvironmentVariables(Application $application)); in that method check
$this->envImported and empty $this->envExampleVars early-return, run the
transaction to create EnvironmentVariable records using
$application->getMorphClass() and $application->id, and replace the in-place
blocks in GithubPrivateRepositoryDeployKey::submit(),
GithubPrivateRepository::submit(), and PublicGitRepository::submit() with a call
to this trait method; additionally validate each env key before creating (ensure
keys match /^[A-Z_][A-Z0-9_]*$/i after trimming/replacement) and skip or log
malformed entries to avoid storing invalid variables.
|
|
||
| if ($this->branchFound) { | ||
| $this->detectRepository(); | ||
| } | ||
| } catch (\Throwable $e) { | ||
| if ($this->rate_limit_remaining == 0) { | ||
| $this->selectedBranch = $this->git_branch; | ||
| $this->branchFound = true; | ||
| $this->detectRepository(); | ||
|
|
||
| return; | ||
| } | ||
| if (! $this->branchFound && $this->git_branch === 'main') { | ||
| try { | ||
| $this->git_branch = 'master'; | ||
| $this->getBranch(); | ||
| if ($this->branchFound) { | ||
| $this->detectRepository(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Detection runs on every branch resolution path — including unverified fallbacks.
detectRepository() is called three times in loadBranch(): after successful branch find (line 189), in the rate-limit fallback (line 195), and after the main→master fallback (line 204). In the rate-limit case (line 192-197), the branch hasn't been verified to exist — detection will clone and potentially fail silently if the branch is wrong.
Consider gating detection on $this->branchFound consistently, or extracting this to a single call after all branch resolution logic completes. Even a Terminator doesn't fire three times when one precise shot will do.
♻️ Suggested consolidation
$this->selectedBranch = $this->git_branch;
-
- if ($this->branchFound) {
- $this->detectRepository();
- }
} catch (\Throwable $e) {
if ($this->rate_limit_remaining == 0) {
$this->selectedBranch = $this->git_branch;
$this->branchFound = true;
- $this->detectRepository();
return;
}
if (! $this->branchFound && $this->git_branch === 'main') {
try {
$this->git_branch = 'master';
$this->getBranch();
- if ($this->branchFound) {
- $this->detectRepository();
- }
} catch (\Throwable $e) {
return handleError($e, $this);
}
} else {
return handleError($e, $this);
}
}
+
+ if ($this->branchFound) {
+ $this->detectRepository();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Livewire/Project/New/PublicGitRepository.php` around lines 187 - 205, The
code currently calls detectRepository() multiple times in loadBranch() (after
the initial successful branch find, inside the rate-limit fallback, and after
the main→master fallback); change the flow so detectRepository() is invoked
exactly once after branch resolution completes and only when $this->branchFound
is true: remove the direct detectRepository() calls inside the try/catch
branches (including the rate-limit block and the main→master fallback), ensure
getBranch() and any branch-fallback logic update
$this->branchFound/$this->git_branch, and add a single conditional call to
detectRepository() at the end of loadBranch() guarded by if ($this->branchFound)
so detection only runs for a verified branch.
| {{-- Dockerfile selector when multiple detected --}} | ||
| @if ($build_pack === 'dockerfile' && count($detectedDockerfiles) > 1) | ||
| <x-forms.select wire:model.live="selectedDockerfile" label="Dockerfile" | ||
| helper="Multiple Dockerfiles were detected in your repository. Select which one to use."> | ||
| @foreach ($detectedDockerfiles as $df) | ||
| <option value="{{ $df }}">{{ $df }}</option> | ||
| @endforeach | ||
| </x-forms.select> | ||
| @endif |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Dockerfile selector block is duplicated across all three creation flows.
Same comment as the detection results — this @if ($build_pack === 'dockerfile' && count($detectedDockerfiles) > 1) block with its <x-forms.select> is copy-pasted. Extract it into a partial for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@resources/views/livewire/project/new/github-private-repository-deploy-key.blade.php`
around lines 107 - 115, Extract the repeated Dockerfile selector block into a
Blade partial (e.g., create a partial like _dockerfile-selector.blade.php) that
accepts the necessary variables ($build_pack, $detectedDockerfiles, and binds to
selectedDockerfile via wire:model.live) and contains the existing `@if`
($build_pack === 'dockerfile' && count($detectedDockerfiles) > 1) ...
<x-forms.select> ... `@endif` markup; then replace the duplicated blocks in the
three creation flows with a single include (e.g.,
`@include`('...._dockerfile-selector', ['build_pack' => $build_pack,
'detectedDockerfiles' => $detectedDockerfiles, 'selectedDockerfile' =>
$selectedDockerfile])) so the UI logic remains identical but maintained in one
place.
|
|
||
| {{-- Dockerfile selector when multiple detected --}} | ||
| @if ($build_pack === 'dockerfile' && count($detectedDockerfiles) > 1) | ||
| <x-forms.select wire:model.live="selectedDockerfile" label="Dockerfile" | ||
| helper="Multiple Dockerfiles were detected in your repository. Select which one to use."> | ||
| @foreach ($detectedDockerfiles as $df) | ||
| <option value="{{ $df }}">{{ $df }}</option> | ||
| @endforeach | ||
| </x-forms.select> | ||
| @endif |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Dockerfile selector: consider also showing it when exactly one Dockerfile is detected.
Currently, the dropdown only appears when count($detectedDockerfiles) > 1. When exactly one Dockerfile is detected and is not at the default location (Dockerfile at root), the user has no visibility into which file was auto-selected. Consider showing the selector (or at least a read-only indicator) when count($detectedDockerfiles) >= 1 so users know what file will be used — like showing someone which taco they're about to eat before they bite in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/livewire/project/new/github-private-repository.blade.php`
around lines 125 - 134, The Dockerfile selector currently only renders when
count($detectedDockerfiles) > 1; update the conditional in the Blade view to
render the selector (or a read-only indicator) when count($detectedDockerfiles)
>= 1 so users can see which Dockerfile was chosen when exactly one is detected;
locate the block using the $build_pack === 'dockerfile' check and the
$detectedDockerfiles array and ensure the <x-forms.select> (bound to
wire:model.live="selectedDockerfile") is shown for a single detected file (or
replace with a disabled/read-only control showing the single
$detectedDockerfiles[0] if you prefer not to allow changing it).
| <input type="text" class="w-2/3 input" | ||
| wire:model.defer="envExampleVars.{{ $key }}" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Livewire 3 wire:model.defer deprecated modifier
💡 Result:
In Livewire 3, wire:model.defer is effectively deprecated/obsolete because wire:model is deferred by default (it only syncs during a network request like submit/click, not on every keystroke). So you should replace:
wire:model.defer="foo"→wire:model="foo"- (If you want Livewire 2’s old default “update as you type”)
wire:model="foo"→wire:model.live="foo" wire:model.lazy="foo"→wire:model.blur="foo"(or.change;.lazyis now an alias for.change)
Sources: Livewire 3 upgrade guide and wire:model docs. [1] [2]
Replace deprecated wire:model.defer with wire:model — in Livewire 3, defer is the default behavior.
In Livewire 3, wire:model syncs on network requests by default (old .defer behavior). The .defer modifier is obsolete and should be removed. If you need live/reactive updates on every keystroke, use wire:model.live instead.
🔧 Proposed fix
- wire:model.defer="envExampleVars.{{ $key }}" />
+ wire:model="envExampleVars.{{ $key }}" />(Unlike serverless garbage that changes APIs every Tuesday, Livewire's defaults actually make sense — server-side state, zero JavaScript headaches. That's the way. 🌮)
📝 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.
| <input type="text" class="w-2/3 input" | |
| wire:model.defer="envExampleVars.{{ $key }}" /> | |
| <input type="text" class="w-2/3 input" | |
| wire:model="envExampleVars.{{ $key }}" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@resources/views/livewire/project/new/partials/env-import-modal.blade.php`
around lines 44 - 45, Replace the deprecated Livewire modifier by removing
".defer" on the input binding: update the input using wire:model.defer in the
env import modal to use wire:model instead (the binding is on envExampleVars.{{
$key }}), i.e., change the attribute on the <input> in
new/partials/env-import-modal.blade.php from wire:model.defer="envExampleVars.{{
$key }}" to wire:model="envExampleVars.{{ $key }}".
| test('parseOutput handles complete detection output', function () { | ||
| $output = json_encode([ | ||
| 'dockerfiles' => ['Dockerfile', 'apps/api/Dockerfile'], | ||
| 'dockerComposeFiles' => ['docker-compose.yml'], | ||
| 'envFiles' => ['.env.example' => "APP_NAME=MyApp\nAPP_ENV=production\nDB_HOST=localhost\nDB_PORT=5432"], | ||
| 'dockerfilePorts' => ['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080], | ||
| ]); | ||
|
|
||
| $detector = new RepositoryDetector( | ||
| repositoryUrl: 'https://github.com/test/repo', | ||
| branch: 'main', | ||
| baseDirectory: '/', | ||
| serverId: 1, | ||
| teamId: 1, | ||
| ); | ||
|
|
||
| $reflection = new ReflectionClass($detector); | ||
| $method = $reflection->getMethod('parseOutput'); | ||
|
|
||
| $result = $method->invoke($detector, $output); | ||
|
|
||
| expect($result->dockerfiles)->toBe(['Dockerfile', 'apps/api/Dockerfile']) | ||
| ->and($result->dockerComposeFiles)->toBe(['docker-compose.yml']) | ||
| ->and($result->envFiles)->toHaveKey('.env.example') | ||
| ->and($result->envFiles['.env.example'])->toContain('APP_NAME=MyApp') | ||
| ->and($result->dockerfilePorts)->toBe(['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080]) | ||
| ->and($result->getSuggestedBuildPack())->toBe(BuildPackTypes::DOCKERCOMPOSE); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract repeated setup into a helper — DRY like a good taco shell. 🌮
Every test case duplicates the RepositoryDetector instantiation and ReflectionClass / getMethod boilerplate. In Pest, you can use beforeEach or a helper closure to eliminate this repetition across all 7 tests.
♻️ Proposed refactor: extract shared setup
Add this at the top of the file (after the use statements) and simplify each test:
use App\Enums\BuildPackTypes;
use App\Services\RepositoryDetector;
+
+beforeEach(function () {
+ $this->detector = new RepositoryDetector(
+ repositoryUrl: 'https://github.com/test/repo',
+ branch: 'main',
+ baseDirectory: '/',
+ serverId: 1,
+ teamId: 1,
+ );
+
+ $reflection = new ReflectionClass($this->detector);
+ $this->parseOutput = $reflection->getMethod('parseOutput');
+});
test('parseOutput handles complete detection output', function () {
$output = json_encode([
'dockerfiles' => ['Dockerfile', 'apps/api/Dockerfile'],
'dockerComposeFiles' => ['docker-compose.yml'],
'envFiles' => ['.env.example' => "APP_NAME=MyApp\nAPP_ENV=production\nDB_HOST=localhost\nDB_PORT=5432"],
'dockerfilePorts' => ['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080],
]);
- $detector = new RepositoryDetector(
- repositoryUrl: 'https://github.com/test/repo',
- branch: 'main',
- baseDirectory: '/',
- serverId: 1,
- teamId: 1,
- );
-
- $reflection = new ReflectionClass($detector);
- $method = $reflection->getMethod('parseOutput');
-
- $result = $method->invoke($detector, $output);
+ $result = $this->parseOutput->invoke($this->detector, $output);
expect($result->dockerfiles)->toBe(['Dockerfile', 'apps/api/Dockerfile'])Apply the same pattern to the remaining 6 tests.
📝 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.
| test('parseOutput handles complete detection output', function () { | |
| $output = json_encode([ | |
| 'dockerfiles' => ['Dockerfile', 'apps/api/Dockerfile'], | |
| 'dockerComposeFiles' => ['docker-compose.yml'], | |
| 'envFiles' => ['.env.example' => "APP_NAME=MyApp\nAPP_ENV=production\nDB_HOST=localhost\nDB_PORT=5432"], | |
| 'dockerfilePorts' => ['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080], | |
| ]); | |
| $detector = new RepositoryDetector( | |
| repositoryUrl: 'https://github.com/test/repo', | |
| branch: 'main', | |
| baseDirectory: '/', | |
| serverId: 1, | |
| teamId: 1, | |
| ); | |
| $reflection = new ReflectionClass($detector); | |
| $method = $reflection->getMethod('parseOutput'); | |
| $result = $method->invoke($detector, $output); | |
| expect($result->dockerfiles)->toBe(['Dockerfile', 'apps/api/Dockerfile']) | |
| ->and($result->dockerComposeFiles)->toBe(['docker-compose.yml']) | |
| ->and($result->envFiles)->toHaveKey('.env.example') | |
| ->and($result->envFiles['.env.example'])->toContain('APP_NAME=MyApp') | |
| ->and($result->dockerfilePorts)->toBe(['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080]) | |
| ->and($result->getSuggestedBuildPack())->toBe(BuildPackTypes::DOCKERCOMPOSE); | |
| }); | |
| beforeEach(function () { | |
| $this->detector = new RepositoryDetector( | |
| repositoryUrl: 'https://github.com/test/repo', | |
| branch: 'main', | |
| baseDirectory: '/', | |
| serverId: 1, | |
| teamId: 1, | |
| ); | |
| $reflection = new ReflectionClass($this->detector); | |
| $this->parseOutput = $reflection->getMethod('parseOutput'); | |
| }); | |
| test('parseOutput handles complete detection output', function () { | |
| $output = json_encode([ | |
| 'dockerfiles' => ['Dockerfile', 'apps/api/Dockerfile'], | |
| 'dockerComposeFiles' => ['docker-compose.yml'], | |
| 'envFiles' => ['.env.example' => "APP_NAME=MyApp\nAPP_ENV=production\nDB_HOST=localhost\nDB_PORT=5432"], | |
| 'dockerfilePorts' => ['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080], | |
| ]); | |
| $result = $this->parseOutput->invoke($this->detector, $output); | |
| expect($result->dockerfiles)->toBe(['Dockerfile', 'apps/api/Dockerfile']) | |
| ->and($result->dockerComposeFiles)->toBe(['docker-compose.yml']) | |
| ->and($result->envFiles)->toHaveKey('.env.example') | |
| ->and($result->envFiles['.env.example'])->toContain('APP_NAME=MyApp') | |
| ->and($result->dockerfilePorts)->toBe(['Dockerfile' => 3000, 'apps/api/Dockerfile' => 8080]) | |
| ->and($result->getSuggestedBuildPack())->toBe(BuildPackTypes::DOCKERCOMPOSE); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/RepositoryDetectorTest.php` around lines 6 - 33, The tests repeat
creating a RepositoryDetector and reflecting its parseOutput method; extract
that setup into a shared beforeEach so each test reuses it: initialize the
RepositoryDetector (repositoryUrl, branch, baseDirectory, serverId, teamId) and
store it plus the ReflectionClass/getMethod('parseOutput') result into
properties (e.g., $this->detector and $this->parseOutputMethod) in a beforeEach
closure, then update each test to call
$this->parseOutputMethod->invoke($this->detector, $output) instead of recreating
the objects; this removes duplicated boilerplate across all tests while keeping
assertions intact.
| test('parseOutput handles dockerfile without EXPOSE', function () { | ||
| $output = json_encode([ | ||
| 'dockerfiles' => ['Dockerfile'], | ||
| 'dockerComposeFiles' => [], | ||
| 'envFiles' => (object) [], | ||
| 'dockerfilePorts' => ['Dockerfile' => null], | ||
| ]); | ||
|
|
||
| $detector = new RepositoryDetector( | ||
| repositoryUrl: 'https://github.com/test/repo', | ||
| branch: 'main', | ||
| baseDirectory: '/', | ||
| serverId: 1, | ||
| teamId: 1, | ||
| ); | ||
|
|
||
| $reflection = new ReflectionClass($detector); | ||
| $method = $reflection->getMethod('parseOutput'); | ||
|
|
||
| $result = $method->invoke($detector, $output); | ||
|
|
||
| expect($result->dockerfiles)->toBe(['Dockerfile']) | ||
| ->and($result->dockerfilePorts)->toBe(['Dockerfile' => null]) | ||
| ->and($result->getSuggestedBuildPack())->toBe(BuildPackTypes::DOCKERFILE); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing edge case: port value as string in JSON.
The parseOutput method (Line 114 in RepositoryDetector.php) checks is_int($port) but JSON-decoded numeric strings will already be int from json_decode. However, if the shell EXPOSE extraction ever produces a string representation (e.g. "3000" as a JSON string rather than number), is_int will return false and the port will be set to null. Consider adding a test where dockerfilePorts has a string port value like "3000" to confirm the expected behavior.
🧪 Proposed additional test case
test('parseOutput handles string port values gracefully', function () {
$output = json_encode([
'dockerfiles' => ['Dockerfile'],
'dockerComposeFiles' => [],
'envFiles' => (object) [],
'dockerfilePorts' => ['Dockerfile' => '3000'], // string, not int
]);
$result = $this->parseOutput->invoke($this->detector, $output);
// Currently this would be null due to is_int check — document or fix this behavior
expect($result->dockerfilePorts['Dockerfile'])->toBeNull();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/RepositoryDetectorTest.php` around lines 63 - 87, The parseOutput
logic in RepositoryDetector currently treats dockerfile port values as valid
only when is_int($port), causing numeric strings like "3000" to be dropped;
update the parseOutput method to accept numeric strings too (e.g., use
is_int($port) || (is_string($port) && ctype_digit($port)) and cast to
(int)$port) when populating dockerfilePorts/SuggestedBuildPack, and add a unit
test similar to the proposed 'parseOutput handles string port values gracefully'
to assert that a string "3000" is recognized and stored as an integer port
rather than null.
|
Hi @adiologydev! 👋 It appears to us that you are adding a new feature to Coolify. Coolify Docs Repository: https://github.com/coollabsio/coolify-docs |
Will do once the PR gets approved :) |
3 participants
Changes
Notes
I think the running commands via jq works, although it can be hard to maintain. I can build a custom parser based on echo as well to simplify it a bit.
To avoid cloning, we can use GitHub/GitLab API, but this means that other providers won't work, so this temp cloning is the preferred method.
Open to change suggestions.
Category
Screenshots or Video (if applicable)
AI Usage
Steps to Test
Contributor Agreement
Important