fix(e2e): assert path URLs, not hash, now the router runs in history mode - #663
Merged
Merged
Conversation
…mode Six settings-roadmap tests failed on the URL alone: Expected pattern: /#\/features-roadmap/ Received string: "http://localhost:8080/apps/larpinq/features-roadmap" The router moved to history mode (createWebHistory, src/main.js), so an in-app route is a real path with no '#'. The helper still demanded a hash, so it failed before ever reaching the .app-content assertion that is the real readiness gate. The app was right; the assertion was describing the old router. Its docblock still described 'mode: hash (fleet #133 deep-link fix)', which had stopped being true. Updated with it, because a stale comment is how the next person re-learns this the hard way. The new pattern is anchored at the end, so /features-roadmap cannot be satisfied by some longer route that merely contains it. Verified against a live instance rather than reasoned about: the file now reports 6 passed.
Contributor
Quality Report — ConductionNL/larpinq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ❌ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 105/105 | |||
| npm | ✅ | ✅ 525/525 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-09-01 17:24 UTC
Download the full PDF report from the workflow artifacts.
This was referenced Sep 1, 2026
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
Formatting only. #663 landed this file unformatted, so Frontend Check (format) fails on development and on every branch that touches the app — it is not diff-scoped to the lines a PR changed, only to the files.
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
CodeQL, high severity: js/incomplete-sanitization. #663 built the URL pattern with route.replace(/\//g, '\\/'), which escapes forward slashes and leaves '.', '?', '+', '(' and backslash live in the pattern. Nothing is exploitable today — every route in this file is a literal — but a route containing '.' would silently match more than it names, which is the same class of quiet wrongness openRoute() was just fixed for. Verified the escaped pattern still matches its own route for '/game-settings', '/features-roadmap', '/a.b' and '/x(y)', and that '/a.b' no longer matches '/axb'. Live run: 6 passed.
rubenvdlinde
added a commit
that referenced
this pull request
Sep 1, 2026
…d clear development (#666) * fix(routes): stop routing health and metrics larpinq cannot serve, and clear development larpinq's development has been red all day on three checks. Two of them are mine, from #651/#653.⚠️ gate-14 (route-reachability): "2 unrouted method(s) or wrong-target route(s)". The gate was RIGHT, and the finding is a latent fatal. #653's class_exists() fallback reproduces Routes::standard()'s output locally so larpinq still routes when OpenRegister is absent — and it copied `metrics#index` and `health#index` along with the rest. larpinq ships neither controller: lib/Controller holds Characters, Dashboard, Events, Preferences, Settings and Setup, and nothing else. Those two work on a normal instance because the branch ABOVE the fallback calls Routes::standard(), and OpenRegister's AppHost aliases the generic Metrics/Health controllers onto larpinq's conventional class names. Verified: /apps/larpinq/api/health answers 200 application/json and /api/metrics answers 200 Prometheus text. But the fallback runs ONLY when OpenRegister is absent, and then nothing aliases anything — so those two routes would have pointed at classes that do not exist, and a request to either would fatal rather than 404. Removed, with the reasoning written where the next person will copy the block. gate-25 (contract-coverage): `dashboard#catchAll` is a new public endpoint with no test. Covered in DashboardControllerTest, asserting equality with page() rather than merely "returns a response" — catchAll() exists only to delegate, and a delegation that quietly rendered the wrong template would give every deep link a blank page while still answering 200. Same test now guards zaakafhandelapp#619 and openregister#3303. Also aligned the fallback's catch-all with Routes::standard()'s `(?!api/)` lookahead. Without it a bare `.+` matches slashes and swallows unmatched `api/...` paths, answering the SPA shell at HTTP 200 — that is what ate all seventeen of zaakafhandelapp's ZGW resource routes (#619) and it would have behaved identically here whenever the fallback was in use. Third check, and NOT mine: prettier failed on tests/e2e/spec-coverage/settings-roadmap.spec.ts, last touched by #663. One statement that fits on a line after the hash was removed from it. Fixed here rather than left, since it keeps development red for everyone. php -l clean, prettier --check on the FULL glob clean, eslint 0 errors, DashboardControllerTest 5/5. The suite reports 5 errors locally, all `Class "Symfony\Component\HttpFoundation\HeaderUtils" not found` in the Characters/Events PDF download tests — a dev dependency missing from this checkout, present in CI, and untouched by this change. * fix(apphost): make the health/metrics generics explicit, so gate-14 can see them Follow-up on the same branch. gate-14 (route-reachability) went on failing with the identical "2 unrouted method(s) or wrong-target route(s)" after the routes were removed from the fallback, and the reason turned out to be two separate things.⚠️ FIRST, THE GATE WAS READING MY COMMENT. I removed the two routes and explained the removal in a comment that spelled their slugs out (`<controller>` + `#` + `<method>`). gate-14 matches that shape anywhere in appinfo/routes.php, comments included, so it kept reporting both — pointing at controller files that do not exist — long after the routes were gone. The comment is reworded and now warns about exactly this. SECOND, AND THE REAL FINDING. With the comment cleaned the gate still reported both, because it knows Routes::standard()'s canonical set and attributes those routes to any app that calls it. It exempts them only when the app shows, in its own code, that it adopts the generic behind them: either `\OCA\OpenRegister\AppHost\Bootstrap::register()`, or a registerService() of the generic controller in the same file that names it. larpinq did neither. Its /api/health and /api/metrics answer 200 on a normal instance purely because the AppHost stands in under larpinq's conventional class names — a real dependency that nothing in this repository stated. So it is stated now: Application::register() registers GenericHealthController and GenericMetricsController under OCA\Larpinq\Controller\{Health,Metrics}Controller, each behind a class_exists() guard so an instance without OpenRegister degrades instead of fataling — the same discipline as the autoloader prelude above it. Registered individually rather than adopting Bootstrap::register() wholesale, which would also alias dashboard, settings, preferences, repair steps and sections onto generics that do NOT match the controllers larpinq actually ships. Same reasoning and the same shape as shillinq's registerAppHostGenerics(). Gate run on this tree: gate-14 PASS, gate-25 PASS (was FAIL, FAIL). Two gates still fail LOCALLY — gate-22 and gate-53, both on `/pages/25/type` — because the vendored hydra-gates schema here predates the `flow` page type. CI reported neither, so its schema is newer; not touched. php -l clean, phpcs 0 errors, DashboardControllerTest 5/5. * refactor(apphost): extract registerAppHostGenerics from register() phpmd's ExcessiveMethodLength fires at 100 lines and register() landed on exactly 100 with the two registrations inline. Extracted to its own method, which is where procest and shillinq both put theirs — the gates push this decomposition, and the previous commit's comment already pointed at shillinq's registerAppHostGenerics() as the model. No behaviour change. phpmd clean, phpcs 0 errors, gate-14 and gate-25 still PASS, DashboardControllerTest 5/5. --------- Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six
settings-roadmaptests fail on the URL alone:The router moved to history mode (
createWebHistory,src/main.js), so an in-app route is a real path with no#. The helper still demanded a hash, so it failed before ever reaching the.app-contentassertion that is the real readiness gate.The app was right. The assertion was describing the old router.
The stale docblock went too
The helper's comment still read "The router runs in
mode: 'hash'(fleet #133 deep-link fix)" — which had stopped being true. I updated it, because a stale comment is how the next person re-learns this the hard way. It now says history mode, and records why the assertion changed.One deliberate tightening
The new pattern is anchored at the end, so
/features-roadmapcannot be satisfied by some longer route that merely contains it. The old hash pattern was unanchored.Verification
Run against a live instance, not reasoned about:
settings-roadmap.spec.ts