Conversation
📝 WalkthroughWalkthroughAdds a new "TanStack Start" framework entry and two site templates (starter and playground) to configuration. Updates detector and runtime wiring: Packager, Framework, Runtime, and Rendering constructors were changed to accept fewer arguments; new addInput(...) calls and Framework::INPUT_* constants were introduced and used across VCS controller and rendering worker. Detection now handles missing package.json defensively. Bumps utopia-php/detector and utopia-php/vcs versions. Minor test and docker image adjustments and an example comment added to the Screenshot task. No existing entries removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Appwrite/Platform/Tasks/Screenshot.php (1)
5-6: Helpful documentation addition.The example usage comments provide valuable guidance for running the screenshot task and clarify the expected output format. This is particularly useful for a new template like TanStack Start.
One minor note: the example shows a relative path (
public/images/sites/templates/...), but the actual implementation (line 306) uses an absolute path (/usr/src/code/public/images/sites/templates/...). Consider clarifying in the comment whether the path is relative to the repository root or updating it to the full absolute path for precision.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
composer.lockis excluded by!**/*.lockpublic/images/sites/templates/playground-for-tanstack-start-dark.pngis excluded by!**/*.pngpublic/images/sites/templates/playground-for-tanstack-start-light.pngis excluded by!**/*.png
📒 Files selected for processing (6)
app/config/frameworks.php(1 hunks)app/config/templates/site.php(2 hunks)app/controllers/api/vcs.php(8 hunks)composer.json(2 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(1 hunks)src/Appwrite/Platform/Tasks/Screenshot.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/TablesDB)
🔇 Additional comments (7)
app/controllers/api/vcs.php (3)
830-838: LGTM: Detector refactoring follows new pattern.The refactoring from constructor-based input injection to zero-argument constructor with
addInput()calls is correctly implemented and consistent with the new detector API.
843-849: Good: Graceful handling of missing package.json.The try-catch with
FileNotFoundproperly handles repositories without a package.json file, allowing framework detection to continue with file-based detection only.
867-880: LGTM: Comprehensive framework detection including TanStack Start.The framework detector now includes all major frameworks including the newly added TanStack Start. The detector initialization and input feeding pattern is consistent.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
877-883: LGTM: Rendering detector refactoring is consistent.The refactoring correctly applies the new detector pattern: construct with the framework value, then feed files via
addInput()in a loop before callingaddOption()anddetect().app/config/templates/site.php (2)
119-119: Fix typo in outputDirectory.The outputDirectory has an extra space and period:
'./ .output'. This should likely be'./.output'to match the Nuxt configuration and standard directory naming conventions.Apply this diff:
- 'outputDirectory' => './ .output', + 'outputDirectory' => './.output',Likely an incorrect or invalid review comment.
1356-1356: Verification confirms providerVersion 0.5. is correct.*The tanstack-start/starter directory exists in the 0.5.0 release. The version specification in the code is intentional and valid—no changes are needed.
app/config/frameworks.php (1)
205-229: Remove helper script verification request—TanStack Start follows the established pattern used by all other frameworks.The configuration uses the same helper path structure as Analog, Angular, Next.js, Nuxt, SvelteKit, Astro, and Remix (absolute paths for
bundleCommand/envCommand, relative paths forstartCommand). Since these existing frameworks presumably work correctly, TanStack Start's identical pattern is not a concern specific to this change. The helpers are either provisioned at runtime/deployment or this is a pre-existing consideration that applies uniformly across all frameworks.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/vcs.php(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (6)
app/controllers/api/vcs.php (6)
32-45: LGTM!The new framework detector imports and
FileNotFoundexception import align with the expanded framework detection capabilities and graceful error handling.Also applies to: 69-69
830-838: LGTM!The refactored Packager initialization using zero-argument constructor with
addInput()calls aligns with the new detector API pattern.
843-878: LGTM!The framework detection logic correctly handles missing package.json files and uses the new detector API with typed inputs (
Framework::INPUT_PACKAGESandFramework::INPUT_FILE). The expanded framework options including TanStackStart align with the PR objectives.
913-936: LGTM!The runtime detection correctly uses the new constructor signature and conditionally feeds inputs based on the detection strategy type.
1031-1039: LGTM!The Packager initialization is consistent with the first occurrence and follows the new API pattern.
1044-1072: LGTM!The framework detection logic is consistent with the first occurrence, using the same graceful error handling and detector API patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/controllers/api/vcs.php (1)
1097-1106: Fix inconsistent Runtime constructor signature.Line 1097 still uses the old constructor signature
new Runtime($strategy === Strategy::LANGUAGES ? $languages : $files, $strategy, $packager)which conflicts with the refactored API that expects inputs to be added viaaddInput()(as correctly done at lines 913-923).Apply this diff to fix the constructor signature:
- $detector = new Runtime($strategy === Strategy::LANGUAGES ? $languages : $files, $strategy, $packager); + $detector = new Runtime($strategy, $packager); if ($strategy === Strategy::LANGUAGES) { foreach ($languages as $language) { $detector->addInput($language);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/config/templates/site.php(3 hunks)app/controllers/api/vcs.php(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: Benchmark
🔇 Additional comments (9)
app/config/templates/site.php (2)
119-128: LGTM!The TanStack Start framework configuration is consistent with other SSR frameworks (NextJS, Nuxt, Remix). The
outputDirectoryof./.outputmatches Nuxt's configuration, and the emptyfallbackFileis appropriate for SSR adapters.
1345-1363: Confirm ifproviderVersion: 0.5.*is intentional for TanStack Start.This template is the only playground using
0.5.*; all other playgrounds (lynx, astro, remix, nextjs, flutter, vite, angular, analog, svelte, react, vue, nuxt, react-native) use0.3.*. Verify the TanStack template exists at version 0.5.0 in thetemplates-for-sitesrepository or align this with the other playgrounds' version.app/controllers/api/vcs.php (7)
32-45: LGTM!The new framework detector imports (Analog, Angular, Lynx, React, ReactNative, Svelte, TanStackStart, Vue) are correctly added and align with the detector options configured later in the file.
830-833: LGTM!The Packager constructor has been correctly refactored to use the zero-argument pattern with incremental
addInput()calls, as described in the PR changes.
843-849: LGTM!The graceful handling of missing
package.jsonfiles is appropriate. Detection can proceed with file-based patterns even when package metadata is unavailable.
858-862: LGTM!The Framework detector refactoring correctly uses the new constructor signature with
$packagerand feeds inputs incrementally viaaddInput()with the appropriate input type constants (INPUT_PACKAGESandINPUT_FILE).
865-878: LGTM!The expanded set of framework detector options now includes TanStack Start and other popular frameworks, providing better framework detection coverage.
913-923: LGTM!The Runtime detector refactoring correctly uses the new constructor signature with only
$strategyand$packagerparameters, then feeds inputs viaaddInput()based on the detection strategy.
1031-1072: LGTM!The detector initialization pattern is correctly duplicated here for batch repository processing, matching the single-repository detection logic at lines 830-878. The refactored API usage is consistent across both code paths.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
public/images/sites/templates/starter-for-tanstack-start-dark.pngis excluded by!**/*.pngpublic/images/sites/templates/starter-for-tanstack-start-light.pngis excluded by!**/*.png
📒 Files selected for processing (2)
app/config/frameworks.php(1 hunks)app/config/templates/site.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/config/frameworks.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
🔇 Additional comments (2)
app/config/templates/site.php (2)
119-128: LGTM! Framework configuration is consistent.The TanStack Start framework configuration follows the established pattern for SSR frameworks (Next.js, Remix, Nuxt, Analog) with appropriate build commands and output directory.
968-1011: LGTM! Starter template is well-structured.The TanStack Start starter template follows the established pattern with appropriate VITE_APPWRITE_* environment variables consistent with other Vite-based frameworks (Remix, Analog, Vue, React).
Meldiron
left a comment
Meldiron
left a comment
There was a problem hiding this comment.
self-reviewed, looks good
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/Services/Sites/SitesCustomClientTest.php (1)
125-125: Remove the unused$hostnamevariable.The
$hostnamevariable is retrieved but no longer used after the assertion changes on lines 134-135.Apply this diff to remove the unused variable:
- $hostname = System::getEnv('_APP_DOMAIN') ?: ''; $this->assertEquals(200, $template['headers']['status-code']);
🧹 Nitpick comments (1)
tests/e2e/Services/Sites/SitesCustomClientTest.php (1)
134-135: Consider the trade-off of weakened URL validation.The assertions now use
assertStringContainsStringinstead of exact URL matching. While this makes the test more resilient to hostname configuration differences, it no longer validates the complete URL structure (protocol, hostname, or that the path is in the correct position).If stricter validation is desired, consider checking both the URL structure and the path:
$this->assertStringStartsWith('http', $template['body']['screenshotDark']); $this->assertStringContainsString('/images/sites/templates/starter-for-react-dark.png', $template['body']['screenshotDark']); $this->assertStringStartsWith('http', $template['body']['screenshotLight']); $this->assertStringContainsString('/images/sites/templates/starter-for-react-light.png', $template['body']['screenshotLight']);Alternatively, if the current approach is intentional for flexibility, the change is acceptable as-is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
app/controllers/api/vcs.php(8 hunks)composer.json(2 hunks)docker-compose.yml(1 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(1 hunks)tests/e2e/Services/Sites/SitesCustomClientTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (12)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
876-879: LGTM! Clean refactor to the new Rendering API.The Rendering constructor signature has been correctly updated to accept only the framework parameter, with file inputs now fed incrementally via
addInput($file). This aligns with the broader detector API refactoring across the codebase.app/controllers/api/vcs.php (10)
32-45: LGTM! New framework detectors and defensive exception handling added.The additional framework detector imports (Analog, Angular, Lynx, React, ReactNative, Svelte, TanStackStart, Vue) and the FileNotFound exception import support the expanded framework detection capabilities and defensive handling of missing package.json files.
Also applies to: 69-69
830-833: LGTM! Packager initialization updated to new API.The Packager constructor signature has been correctly updated to accept no arguments, with file inputs now fed incrementally via
addInput($file)calls. This follows the new incremental input pattern.
843-849: LGTM! Defensive handling for missing package.json.The try-catch block gracefully handles repositories without a package.json file, allowing framework detection to continue with empty package content. This is good defensive coding that prevents detection failures for projects that may not have this file.
858-862: LGTM! Framework detector updated to new API with input type constants.The Framework constructor signature has been correctly updated to accept only the packager parameter. The new
Framework::INPUT_PACKAGESandFramework::INPUT_FILEconstants properly distinguish between package content and file names when feeding inputs viaaddInput().
864-878: LGTM! Comprehensive framework detection expanded.The framework detector now supports additional frameworks including TanStack Start (per PR objectives), along with Analog, Angular, Lynx, React, ReactNative, Svelte, and Vue. This significantly expands framework detection capabilities.
913-923: LGTM! Runtime detector constructor signature correctly updated.The Runtime constructor now correctly accepts only
$strategyand$packagerparameters, with inputs fed viaaddInput()based on the detection strategy. The strategy-based conditional feeding (languages for LANGUAGES strategy, files for others) is logically correct.
1031-1034: LGTM! Consistent Packager initialization in batch processing.The Packager initialization in the batch processing context follows the same correct pattern as the single repository detection (lines 830-833), ensuring consistency across the codebase.
1044-1050: LGTM! Consistent defensive handling in batch processing.The try-catch pattern for missing package.json files is consistently applied in the batch processing context, matching the implementation in single repository detection (lines 843-849).
1052-1072: LGTM! Consistent Framework detector implementation in batch processing.The Framework detector initialization and option configuration in the batch processing context correctly mirrors the implementation in single repository detection (lines 858-878), ensuring consistent behavior across both code paths.
1097-1106: LGTM! Runtime detector correctly updated in batch processing.The Runtime constructor signature has been correctly updated to match the new API (previously flagged in past reviews). The implementation now consistently uses
new Runtime($strategy, $packager)with strategy-based input feeding, matching the single repository detection pattern (lines 913-923).docker-compose.yml (1)
969-969: Version verification complete—no issues found.The openruntimes/executor:0.11.4 version has been verified:
- Confirmed to exist on Docker Hub with digest sha256:ae044926dd062e3a3c4267fa77c15657cf73fc92deaba381db093678fa7f8db3
- Confirmed as a stable release (not a prerelease) on GitHub, published today (2025-10-24)
- Part of normal incremental progression (0.11.0 → 0.11.4)
- No known security advisories identified
The version bump is legitimate and stable.
2 participants
What does this PR do?
Test Plan
Manual QA
Related PRs and Issues
x
Checklist