Skip to content

chore(dev tool): route drift script - #13849

Open
brainbicycle wants to merge 29 commits into
mainfrom
brian/route-drift-script
Open

chore(dev tool): route drift script#13849
brainbicycle wants to merge 29 commits into
mainfrom
brian/route-drift-script

Conversation

@brainbicycle

@brainbicycle brainbicycle commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

This PR resolves []

Description

Problem:
Eigen supports universal links + deep links, so its route table
(src/app/Navigation/routes.tsx) is
meant to mirror the web app's routes
(artsy/force src/Apps/*/*Routes.tsx) — with
some intentional exceptions. Nothing enforces that, so the two drift, and drift
produces linking bugs: e.g.: an artsy.net URL that should open a native screen
silently opens inside a webview instead. Similarly Android and iOS linking are supposed to be consistent but are configured distinctly, on iOS all links open the app by default with a denylist in our universal links config. On Android link prefixes must be added explicitly to manifest so there is another area things can drift.

Adds a route drift report script that can be periodically run to review our routing and universal linking alignment across force, android and iOS. See README in script folder for more details.

~/C/eigen (brian/route-drift-script|✔) $ yarn route-drift
Fetching force route files via the GitHub API…
  found 62 *Routes.tsx files
  extracted 190 canonical force route templates
  loaded 21 AASA universal-link exclusions
  loaded 66 Android manifest path rules

Report written to /Users/brianbeckerle/Code/eigen/scripts/route-drift/route-drift-report.md
Force routes: 190 total — 61 native, 129 webview (13 AASA-excluded, 99 actionable)
Eigen orphan routes (no force match): 69
Android: 5 manifest↔AASA conflicts, 9 native routes missing from manifest

PR Checklist

  • I have tested my changes on the following platforms:
    • Android.
    • iOS.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos at least on Android, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • route drift report script - brian

Need help with something? Have a look at our docs, or get in touch with us.

@brainbicycle brainbicycle self-assigned this Jul 29, 2026
@brainbicycle
brainbicycle marked this pull request as ready for review July 29, 2026 17:11
@artsyit

artsyit commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Dev changes (route drift report script - brian - brainbicycle)

Generated by 🚫 dangerJS against de43e0e

Comment thread scripts/route-drift/parseAASA.ts
Comment thread scripts/route-drift/generate.ts
Comment thread scripts/route-drift/generate.ts
Comment thread scripts/route-drift/parseForceRoutes.ts
Comment thread scripts/route-drift/parseForceRoutes.ts Outdated
Comment thread scripts/route-drift/parseAASA.ts Outdated
Comment thread scripts/route-drift/parseForceRoutes.ts
Comment thread scripts/route-drift/generate.ts Outdated
@github-actions github-actions Bot deleted a comment from claude Bot Jul 29, 2026
@brainbicycle
brainbicycle requested review from a team July 29, 2026 18:03
Comment thread scripts/route-drift/parseEigenRoutes.ts Outdated
Comment thread scripts/route-drift/generate.ts Outdated
anandaroop
anandaroop previously approved these changes Jul 29, 2026

@anandaroop anandaroop left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a helpful addition!

Is the plan to wire this up in CI as well? (I see that the --strict flag implies that, but I didn't see any config for that yet in this PR.)

| Yes | Yes | **exclude** (`NOT`) | flow wins; screen still reachable via `artsy://` |
| No | Yes | **exclude** (`NOT`) | keep the user in the web flow |
| No | No — content worth having | allow → in-app webview, or build a screen | this is "actionable drift" in the report |
| No | No — web-only (admin, SEO, legal) | either | exclude to avoid a degraded webview, or allowlist |

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (non-blocking): Is an in-app webview necessarily degraded, or does that depend on which route?

Comment thread scripts/route-drift/allowlist.json Outdated
@brainbicycle

Copy link
Copy Markdown
Contributor Author

Looks like a helpful addition!

Is the plan to wire this up in CI as well? (I see that the --strict flag implies that, but I didn't see any config for that yet in this PR.)

That was initially the plan, possibly on modifying routes.tsx but there is a fair bit of clean up that needs to happen for that to work so removing for now! and we can just run locally every so often and once we get to good spot maybe add to CI.

@github-actions github-actions Bot deleted a comment from claude Bot Jul 30, 2026
Comment thread scripts/route-drift/generate.ts
Comment thread scripts/route-drift/parseForceRoutes.ts
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@github-actions github-actions Bot deleted a comment from claude Bot Jul 30, 2026
Comment thread scripts/route-drift/generate.ts
Comment thread scripts/route-drift/parseAndroidManifest.ts
Comment thread scripts/route-drift/generate.ts
Comment thread scripts/route-drift/match.ts
Comment thread scripts/route-drift/parseForceRoutes.ts
@github-actions github-actions Bot deleted a comment from claude Bot Jul 30, 2026
Comment thread scripts/route-drift/generate.ts
Comment thread scripts/route-drift/generate.ts Outdated
@github-actions github-actions Bot deleted a comment from claude Bot Jul 30, 2026
Comment thread scripts/route-drift/parseAndroidManifest.ts
Comment thread scripts/route-drift/generate.ts
process.stdout.write(JSON.stringify(results))
}

main()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main() is async and unhandled here (same in parseEigenRoutesHarness.ts, though that one is sync). If it rejects, Node exits non-zero with an unhandled-rejection trace, and execFileSync in the Jest test throws Unexpected end of JSON input from JSON.parse(out) instead of showing the real cause. A .catch that writes the error to stderr and calls process.exit(1) makes the failure legible.

@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a dev-only yarn route-drift script that cross-references eigen's routes.tsx against artsy/force's *Routes.tsx (fetched live via Octokit), artsy.net's AASA NOT list, and the Android manifest App Links allowlist, then writes a markdown report of the drift. The only production change is RouteMatcher.ts switching its AppModule import to import type so match.ts can load the real matcher under tsx without the RN module graph — enforced by checkMatchIsolation.ts plus a subprocess test.

Good call reusing the real RouteMatcher and replicating matchRoute.ts's first-match-wins loop rather than re-implementing matching — I checked getDomainMap.ts and the order/compact behaviour lines up, so classifyURL reflects production. The README's AASA rubric and "Known limitations" section are the part that will keep this useful in six months.


Issues Found

🟡 Important — 1. parseAndroidManifest fails open, silently inverting the report (scripts/route-drift/parseAndroidManifest.ts:29-54)

If no intent-filter declares an ARTSY_HOSTS host, the function returns { rules: [], prefixes: [] } instead of throwing, so generate.ts's try/catch never fires. Check A then reports 0 conflicts and Check B reports every native route as missing from the manifest — roughly 60 false positives, no warning. The NOTE at generate.ts:103-106 claims "the warnings above surface that", but in this case nothing does. fetchAASAExclusions has the same shape of problem: it only throws on !res.ok, so an AASA format change yields patterns: [] and turns every deliberate exclusion into "actionable drift". Inline comment has a suggested guard.

🟢 Suggestion — 2. loadAllowlist only handles missing keys, not malformed ones (scripts/route-drift/generate.ts:225-229)

"ignorePrefixes": {} throws .map is not a function at line 58; an entry missing prefix throws Cannot read properties of undefined (reading 'replace') at line 61. Both bypass the friendly allowlist.json is not valid JSON error right above. Inline suggestion filters to well-formed entries.

🟢 Suggestion — 3. Unhandled async main() (scripts/route-drift/parseForceRoutesHarness.ts:47)

A rejection surfaces in the Jest test as Unexpected end of JSON input rather than the real error. A .catch writing to stderr and calling process.exit(1) makes it legible.

🟢 Suggestion — 4. NON_NATIVE_MODULES is a hardcoded set (scripts/route-drift/match.ts:6)

parseEigenRoutes.ts:111 already derives ReactWebView/ModalWebView from webViewRoute, so the knowledge lives in two places. If someone adds another webview wrapper module, the report will silently count it as native coverage. Not worth blocking on — the README documents the list — but a comment pointing at webViewRoute in routes.tsx would help the next person keep them in sync.


Areas Reviewed

Architecture — Splitting the pure helpers (canonicalize.ts, match.ts) from the typescript/Octokit-importing parsers is what makes most of this testable in-process. The import type constraint on RouteMatcher.ts is a real coupling from a script back onto production code, but checkMatchIsolation.ts and matchIsolation.tests.ts enforce it with a clear failure message, which is the right trade.

Correctness — Spot-checked the tricky bits: the AASA /news/* vs bare /news distinction matches Apple's legacy paths semantics; the isWildcard && prefix.startsWith(base) guard in Check A correctly stops exact /news from conflicting with /newsletter while still catching /identity-verification* against /identity-verification-faq; androidPatternToRegExp maps Android's ./* glob to equivalent regex; expandOptionals terminates on the absent.join("/") || "/" path. matchesRule's non-segment-aware pathPrefix is real Android behaviour and the README calls it out.

SecurityGITHUB_TOKEN from .env.releases follows the existing pattern in scripts/changelog and scripts/utils. No token in report output; the report itself is gitignored.

Testing — Coverage is solid for a dev script: the force/eigen walkers are exercised over inline fixtures (spreads, shorthand path, non-literal path, fetch failure, dedup, no-arrays), toMatcher/matchesRule/classifyURL have table tests, and the isolation guard has its own subprocess test. testMatch: "<rootDir>/**/*.tests.(ts|tsx|js)" picks these up, so they run in CI. Nothing in generate.ts (loadAllowlist, Check A conflict pairing, normalize) is covered — that is where issue 2 lives, and where a couple of table tests would pay off.


Questions for Author

  • The report is gitignored, so drift is only visible to whoever runs the script. Is the plan to wire this into a scheduled CI job that opens an issue/PR, or does it stay manual? The // Worth failing on once this runs in CI comment suggests the former.
  • parseForceRoutesHarness.ts:10-13 notes a scripts/** Jest project with testEnvironment: "node" would remove the ~40min typescript import cost and let both harnesses drop their subprocesses. Deferred here to avoid touching jest.config.js — is that a follow-up you want tracked?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants