Fix Nextcloud 33 compatibility and harmonize workflows#112
Conversation
- Downgrade symfony/string to v7.x in composer.lock for PHP 8.2 compatibility - Update appinfo/info.xml and composer.json to target PHP 8.2 - Fix syntax error and update phpVersion in psalm.xml - Fix PSR-4 namespace in tests/unit/ApplicationTest.php - Harmonize GitHub Actions workflows with consistent hashes and tags - Create AGENTS.md with project-specific agent instructions Co-authored-by: ashcoft <1115854+ashcoft@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThis PR standardizes CI setup-php comments/pins, switches numerous workflows from ChangesRepository maintenance and tooling updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/App.vueParsing error: Unexpected token ViewerInstance 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 |
- Resolve frontend dependency conflict by downgrading jest-watch-typeahead - Use --legacy-peer-deps for npm installation in CI and local dev - Update metadata to target PHP 8.2 and Nextcloud 33 - Add and configure php-cs-fixer and phpstan for static analysis - Harmonize GitHub Actions workflows with consistent hashes and tags - Fix PSR-4 namespace in tests and composer.json - Fix syntax error in psalm.xml - Add AGENTS.md with durable development instructions Co-authored-by: ashcoft <1115854+ashcoft@users.noreply.github.com>
- Resolve frontend dependency conflicts in package.json - Establish PHP quality tools: php-cs-fixer, phpstan, rector - Fix PSR-4 namespace violations in tests - Correct syntax and versioning in psalm.xml - Harmonize GitHub Actions workflows with consistent pinned hashes - Add AGENTS.md for durable development context - Configure stylelint for Vue and CSS components Co-authored-by: ashcoft <1115854+ashcoft@users.noreply.github.com>
- Add stylelint, phpstan, and rector to CI workflows and local scripts - Switch from 'npm ci' to 'npm install --legacy-peer-deps' in all workflows to resolve peer dependency conflicts - Downgrade jest-watch-typeahead to v2.2.2 for Jest 29 compatibility - Fix PSR-4 namespace discovery for unit tests - Correct syntax error in psalm.xml - Add AGENTS.md with development guidelines for AI agents - Establish baseline configurations for phpstan, php-cs-fixer, and stylelint Co-authored-by: ashcoft <1115854+ashcoft@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/appstore-build-publish.yml (1)
117-117: 🏗️ Heavy liftAdd an inline comment explaining why
--legacy-peer-depsis needed.While the rationale is documented in the commit message, the code lacks inline documentation for why
npm install --legacy-peer-depsreplaces the more deterministicnpm ci. This makes the decision less discoverable for future maintainers.Add a brief comment above line 117:
cd ${{ env.APP_NAME }} + # Using --legacy-peer-deps to resolve peer dependency conflicts npm install --legacy-peer-depsConsider also tracking a TODO to resolve the underlying peer dependency conflicts properly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/appstore-build-publish.yml at line 117, Add an inline comment above the npm install step that explains why npm install --legacy-peer-deps is used instead of the deterministic npm ci (i.e., to bypass current peerDependency conflicts in the dependency tree), and include a short TODO note to resolve those peer dependency issues upstream; reference the exact command string (--legacy-peer-deps) and the replaced command name (npm ci) so reviewers can easily find and later remove the workaround..github/workflows/node-test.yml (1)
82-87: ⚡ Quick winUse
npm ci --legacy-peer-depsin CI for deterministic installs.
npm installcan make dependency resolution less reproducible between runs. Since the workflow specifies npm v11 (package.json engines constraint), usenpm ci --legacy-peer-depsto maintain deterministic installs while preserving the peer-deps workaround.Proposed change
- npm install --legacy-peer-deps + npm ci --legacy-peer-deps🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/node-test.yml around lines 82 - 87, Update the CI step named "Install dependencies & build" to use a deterministic install: replace the `npm install` invocation with `npm ci --legacy-peer-deps` while keeping the existing environment variable `CYPRESS_INSTALL_BINARY: 0` and the subsequent `npm run build --if-present` command; this ensures reproducible installs in the workflow step that currently runs under the npm v11 constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/appstore-build-publish.yml:
- Line 117: Add an inline comment above the npm install step that explains why
npm install --legacy-peer-deps is used instead of the deterministic npm ci
(i.e., to bypass current peerDependency conflicts in the dependency tree), and
include a short TODO note to resolve those peer dependency issues upstream;
reference the exact command string (--legacy-peer-deps) and the replaced command
name (npm ci) so reviewers can easily find and later remove the workaround.
In @.github/workflows/node-test.yml:
- Around line 82-87: Update the CI step named "Install dependencies & build" to
use a deterministic install: replace the `npm install` invocation with `npm ci
--legacy-peer-deps` while keeping the existing environment variable
`CYPRESS_INSTALL_BINARY: 0` and the subsequent `npm run build --if-present`
command; this ensures reproducible installs in the workflow step that currently
runs under the npm v11 constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 266edd2e-da7e-4d00-9607-662a7887e4db
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
.github/workflows/appstore-build-publish.yml.github/workflows/command-compile.yml.github/workflows/command-openapi.yml.github/workflows/cypress.yml.github/workflows/documentation.yml.github/workflows/lint-eslint.yml.github/workflows/lint-stylelint.yml.github/workflows/lint-typescript.yml.github/workflows/node-test.yml.github/workflows/npm-audit-fix.yml.github/workflows/npm-build.yml.github/workflows/openapi.yml.github/workflows/phpstan.yml.php-cs-fixer.dist.phpappinfo/routes.phpcomposer.jsoncss/cad-viewer.csslib/AppInfo/Application.phplib/Controller/FileController.phplib/Controller/ViewController.phplib/Listener/LoadViewer.phppackage.jsonphpstan.neonrector.phpsrc/App.vuestylelint.config.jstests/unit/ApplicationTest.php
✅ Files skipped from review due to trivial changes (9)
- appinfo/routes.php
- .github/workflows/phpstan.yml
- lib/Listener/LoadViewer.php
- rector.php
- src/App.vue
- stylelint.config.js
- .php-cs-fixer.dist.php
- lib/Controller/FileController.php
- tests/unit/ApplicationTest.php
ashcoft
deleted the
fix/nc33-compatibility-and-workflows-16460799255086214259
branch
1 participant
This PR addresses the CI failures and ensures compatibility with Nextcloud 33 and PHP 8.2.
Key changes:
symfony/stringwas attempting to usev8.x(requiring PHP 8.4) by downgrading it tov7.xincomposer.lock.appinfo/info.xmlandcomposer.jsonto explicitly target PHP 8.2 and Nextcloud 33.psalm.xmlthat was breaking static analysis.tests/unit/ApplicationTest.phpto comply with PSR-4 as defined incomposer.json.AGENTS.mdto provide durable context and instructions for future AI agent interventions.Verified that
npm install --legacy-peer-depsworks andnpm testpasses. Backend dependencies are now resolvable on PHP 8.2.PR created automatically by Jules for task 16460799255086214259 started by @ashcoft
Summary by CodeRabbit
Chores
Tests
Style
New Features