-
Notifications
You must be signed in to change notification settings - Fork 591
chore(dev tool): route drift script #13849
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
d26e3e3
route drift report script
brainbicycle f5a88d5
route drift script
brainbicycle ca6d4f3
refactor to use oktokit and env
brainbicycle 1d0a5e6
cleanup overly verbose comments
brainbicycle b567769
ignore claude project memory
brainbicycle 41976c8
pr review
brainbicycle bff9852
pr review: clean up unused function, silent empty route fail
brainbicycle 26ed6d0
pr review: use fetch instead of curl
brainbicycle 29c6c0a
pr review: back off gh requests and distinguish rate limited
brainbicycle 70469e5
pr review: use real non-native modules object to avoid drift
brainbicycle b3c99a3
pr review: fix modalwebview parsing
brainbicycle 77fa2d5
pr review: ignore prefix silently could drop routes
brainbicycle 4b9dac8
add tests
brainbicycle e8c0d97
enforcing and testing for isolation
brainbicycle 2a43ec6
pr review: rate limit check that works in v1, v2 oktokit
brainbicycle b4e318f
pr review: remove aasa android match inconsistency
brainbicycle 172abf3
remove strict mode for now
brainbicycle 69e2f14
pr review: silent fails in force parse
brainbicycle fad804c
pr review: deduplicate example path logic
brainbicycle f46f45c
pr review: edge case android manifest failure
brainbicycle 7dada64
add test for force route parsing
brainbicycle 44c86ca
pr review: remove now unnecessary games prefix ignore
brainbicycle 385bef0
Merge branch 'brian/route-drift-script' of https://github.com/artsy/e…
brainbicycle ce6f7a8
type issues
brainbicycle baa7646
pr review: handle malformed allowlist
brainbicycle 0f0650e
pr review: missed object types
brainbicycle bcee5b1
pr review: more test coverage
brainbicycle bb69383
Merge branch 'brian/route-drift-script' of https://github.com/artsy/e…
brainbicycle de43e0e
subtle wildcard bug
brainbicycle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| # Route Drift Report (eigen ↔ artsy/force) | ||
|
|
||
| Eigen supports universal links + deep links, so its route table | ||
| ([`src/app/Navigation/routes.tsx`](../../src/app/Navigation/routes.tsx)) is | ||
| meant to mirror the web app's routes | ||
| ([`artsy/force`](https://github.com/artsy/force) `src/Apps/*/*Routes.tsx`) — with | ||
| some intentional exceptions. Nothing enforces that, so the two drift, and drift | ||
| produces linking bugs: an artsy.net URL that _should_ open a native screen | ||
| silently opens inside a webview instead. | ||
|
|
||
| This tool generates a report answering: | ||
|
|
||
| - **Which force routes does eigen handle natively, and where do they route?** | ||
| - **Which force routes does eigen NOT handle** (they fall through to a webview / | ||
| vanity-URL resolver)? | ||
| - **Which native eigen routes have no force counterpart** (candidate orphans / | ||
| app-only screens)? | ||
| - **Where do the deep-link configs disagree across platforms?** | ||
| - Paths the Android manifest allowlists that iOS AASA excludes (e.g. `/news`). | ||
| - Native eigen screens missing from the Android manifest allowlist (so their | ||
| web links open the browser on Android instead of the app). | ||
|
|
||
| ## Usage | ||
|
|
||
| ```sh | ||
| yarn route-drift # writes scripts/route-drift/route-drift-report.md | ||
| ``` | ||
|
|
||
| Force routes are fetched live from the GitHub API at `main` (via Octokit). A | ||
| `GITHUB_TOKEN` (a token with public repo read access) is read from | ||
| `.env.releases` — or the environment — to avoid rate limits; unauthenticated | ||
| requests cap at 60/hour, which isn't enough to fetch every route file. | ||
|
|
||
| ## How it works | ||
|
|
||
| 1. **Eigen side** ([`parseEigenRoutes.ts`](./parseEigenRoutes.ts)) — statically | ||
| AST-parses `routes.tsx` into an ordered `{ path, name }` list (order matters: | ||
| eigen matches first-match-wins). | ||
| 2. **Force side** ([`parseForceRoutes.ts`](./parseForceRoutes.ts)) — fetches | ||
| every `src/Apps/**/*Routes.tsx`, AST-walks nested `children`, and | ||
| canonicalizes each into a concrete sample URL (params substituted, regex | ||
| escape-hatches stripped, optional params expanded). | ||
| 3. **Matching** ([`match.ts`](./match.ts)) — imports eigen's **real** | ||
| [`RouteMatcher`](../../src/app/system/navigation/utils/RouteMatcher.ts) and | ||
| replicates [`matchRoute`](../../src/app/system/navigation/utils/matchRoute.ts)'s | ||
| first-match-wins loop, so results reflect production behavior rather than a | ||
| re-implementation. A module name of `ReactWebView` / `ModalWebView` / | ||
| `VanityURLEntity` (or no match) = falls through to a webview. | ||
| 4. **AASA** ([`parseAASA.ts`](./parseAASA.ts)) — reads artsy.net's | ||
| [`apple-app-site-association`](https://www.artsy.net/.well-known/apple-app-site-association) | ||
| live and extracts the `NOT` patterns (paths deliberately excluded from | ||
| universal links). Force routes matching these are treated as intentional and | ||
| dropped from actionable drift — an authoritative, self-updating exclusion | ||
| layer on top of the manual allowlist. | ||
| 5. **Android** ([`parseAndroidManifest.ts`](./parseAndroidManifest.ts)) — reads | ||
| the App Links allowlist (`<data>` path rules in the artsy.net autoVerify | ||
| intent-filter) from the local | ||
| [`AndroidManifest.xml`](../../android/app/src/main/AndroidManifest.xml) — no | ||
| network needed. Android has no deny mechanism, so it's an inclusion allowlist; | ||
| the report cross-checks it against both the AASA exclusions and eigen's native | ||
| routes. | ||
| 6. **Report** ([`generate.ts`](./generate.ts)) — writes grouped markdown. | ||
|
|
||
| A webview route is only counted as **actionable drift** when it is _not_ native, | ||
| _not_ suppressed by the allowlist / ignore-prefixes, and _not_ AASA-excluded. | ||
|
|
||
| ## Allowlist | ||
|
|
||
| Intentional divergences go in [`allowlist.json`](./allowlist.json) with a | ||
| `reason`, and are suppressed from the report's actionable sections. | ||
|
|
||
| ## Maintaining the AASA exclusion list (rubric) | ||
|
|
||
| The `NOT` entries in the | ||
| [`apple-app-site-association`](https://www.artsy.net/.well-known/apple-app-site-association) | ||
| file decide which URLs are _deliberately_ kept out of the app: a matching URL | ||
| opens mobile web instead of deep-linking into eigen. The file is served at | ||
| artsy.net by a Cloudflare Worker and maintained in | ||
| [`artsy/artsy-eigen-web-association`](https://github.com/artsy/artsy-eigen-web-association) | ||
| — the `NOT` list lives in | ||
| [`constants.ts`](https://github.com/artsy/artsy-eigen-web-association/blob/main/constants.ts). | ||
| That list drifts too — exclusions get added for a | ||
| reason that later stops applying (e.g. we couldn't render editorial content | ||
| natively, but now we can), or a flow that _should_ be excluded never gets added. | ||
| Review it periodically alongside this report. | ||
|
|
||
| Two **independent** questions decide where a route belongs: | ||
|
|
||
| **1. Does a native screen exist (or should one) for this content?** | ||
| If yes, the route belongs in `routes.tsx` and should **not** be in the AASA | ||
| `NOT` list — we want universal links to open the rich native screen. The report's | ||
| **🔀 Review** section flags violations of this (native screen exists _and_ | ||
| AASA-excluded), e.g. `/news`. | ||
|
|
||
| **2. Is this route part of a web flow that must not be interrupted?** | ||
| Checkout, payment, auth / OAuth callbacks, identity verification. If yes, it | ||
| **should** be in the AASA `NOT` list — bouncing a mid-checkout mobile-web user | ||
| into the app loses their session and context. (Today `/order`/checkout routes are | ||
| _not_ excluded — a candidate to add.) | ||
|
|
||
| Decision table: | ||
|
|
||
| | Native screen exists? | Uninterruptible web flow? | AASA should… | Notes | | ||
| | --------------------- | --------------------------------- | ----------------------------------------- | ------------------------------------------------- | | ||
| | Yes | No | **allow** (not in `NOT`) | native deep-link — the happy path | | ||
| | 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 | | ||
|
|
||
| **Periodic review checklist** (run `yarn route-drift`, then): | ||
|
|
||
| - **🔀 native but AASA-excluded** → likely a stale exclusion; remove it from the | ||
| AASA `NOT` list in `artsy/artsy-eigen-web-association` so links reach the | ||
| native screen. | ||
| - **⚠️ actionable drift** → a web link opens an in-app webview. Build a native | ||
| screen, OR (if it's an uninterruptible flow) add it to the AASA `NOT` list, OR | ||
| allowlist it here if the webview is intentional. | ||
| - **🍎 AASA-excluded** → sanity-check each still makes sense (does the app now | ||
| handle this content?). | ||
|
|
||
| Changing the AASA list requires a PR to `artsy/artsy-eigen-web-association` | ||
| (then the Cloudflare Worker redeploys); changing native coverage is a PR here in | ||
| eigen. | ||
|
|
||
| ## Known limitations | ||
|
|
||
| - **Param names are wildcarded positionally** (`:slug` ≡ `:fairID` ≡ `:id`), so | ||
| matching is by segment shape, not param identity. | ||
| - **Query-param-based routes** and complex regex segments in force are | ||
| approximated; annotate exceptions in the allowlist. | ||
| - **Redirects** (force sometimes 301s, e.g. gene→collection) are not followed — | ||
| a redirected force path may show as "webview" though the target is native. | ||
| - **Android `pathPrefix` is not segment-aware** — `/collect` literally matches | ||
| `/collections` too. The cross-platform check reflects this real behavior, so | ||
| it may surface prefix-overlap pairs (e.g. `/collect` ↔ `/collections`) that are | ||
| technically correct but low-priority. | ||
| - Static parsing can miss routes assembled dynamically; see the "Parser | ||
| warnings" section of the report. | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import { expandOptionals, joinPaths, stripRegex } from "../canonicalize" | ||
|
|
||
| describe("stripRegex", () => { | ||
| it.each([ | ||
| ["exhibitors(.*)?", "exhibitors"], | ||
| ["booths(.*)?", "booths"], | ||
| ["/fair/:slug/exhibitors(.*)?", "/fair/:slug/exhibitors"], | ||
| ["foo(a|b)bar", "foobar"], | ||
| ["a//b", "a/b"], | ||
| ["plain", "plain"], | ||
| ])("strips %p -> %p", (input, expected) => { | ||
| expect(stripRegex(input)).toEqual(expected) | ||
| }) | ||
| }) | ||
|
|
||
| describe("joinPaths", () => { | ||
| it.each([ | ||
| [["/fair", "artworks"], "/fair/artworks"], | ||
| [["/fair/", "/artworks"], "/fair/artworks"], | ||
| [["fair", "artworks"], "/fair/artworks"], | ||
| [["", "artworks"], "/artworks"], | ||
| // index / empty child collapses to the parent prefix | ||
| [["/fair", ""], "/fair"], | ||
| [["/fair", "/"], "/fair"], | ||
| [["", ""], "/"], | ||
| ])("joins %p -> %p", ([prefix, path], expected) => { | ||
| expect(joinPaths(prefix, path)).toEqual(expected) | ||
| }) | ||
| }) | ||
|
|
||
| describe("expandOptionals", () => { | ||
| it("returns the path unchanged when there is no optional param", () => { | ||
| expect(expandOptionals("/collect/:medium")).toEqual(["/collect/:medium"]) | ||
| }) | ||
|
|
||
| it("expands a trailing optional into with- and without-variants", () => { | ||
| expect(expandOptionals("/collect/:medium?")).toEqual(["/collect/:medium", "/collect"]) | ||
| }) | ||
|
|
||
| it("keeps only the present variant for a non-terminal optional", () => { | ||
| expect(expandOptionals("/fair/:slug?/exhibitors")).toEqual(["/fair/:slug/exhibitors"]) | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { classifyURL, compileEigenRoutes } from "../match" | ||
|
|
||
| // match.ts imports only `url` + RouteMatcher (EigenRoute is `import type`), so | ||
| // there's no `typescript` pull-in — this runs in-process, no tsx harness. | ||
| const route = (path: string, name: string) => ({ path, name, order: 0 }) | ||
| const at = (path: string) => `https://www.artsy.net${path}` | ||
|
|
||
| describe("classifyURL", () => { | ||
| it("is first-match-wins — a specific route ordered before the /:slug catch-all wins", () => { | ||
| const specificFirst = compileEigenRoutes([ | ||
| route("/artworks", "Artworks"), | ||
| route("/:slug", "VanityURLEntity"), | ||
| ]) | ||
| expect(classifyURL(at("/artworks"), specificFirst).module).toBe("Artworks") | ||
|
|
||
| // Reverse the order and the catch-all shadows it — proving order decides. | ||
| const catchAllFirst = compileEigenRoutes([ | ||
| route("/:slug", "VanityURLEntity"), | ||
| route("/artworks", "Artworks"), | ||
| ]) | ||
| expect(classifyURL(at("/artworks"), catchAllFirst).module).toBe("VanityURLEntity") | ||
| }) | ||
|
|
||
| it("resolves a native route to its module and marks it native", () => { | ||
| const compiled = compileEigenRoutes([route("/artwork/:artworkID", "Artwork")]) | ||
| expect(classifyURL(at("/artwork/andy-warhol-soup-can"), compiled)).toEqual({ | ||
| module: "Artwork", | ||
| matchedPath: "/artwork/:artworkID", | ||
| isNative: true, | ||
| }) | ||
| }) | ||
|
|
||
| it("classifies the /:slug VanityURLEntity catch-all as non-native", () => { | ||
| const compiled = compileEigenRoutes([route("/:slug", "VanityURLEntity")]) | ||
| expect(classifyURL(at("/some-vanity-slug"), compiled)).toEqual({ | ||
| module: "VanityURLEntity", | ||
| matchedPath: "/:slug", | ||
| isNative: false, | ||
| }) | ||
| }) | ||
|
|
||
| it("matches a trailing-wildcard route across the remaining segments", () => { | ||
| const compiled = compileEigenRoutes([route("/artist/:artistID/*", "Artist")]) | ||
| expect(classifyURL(at("/artist/banksy/auction-results"), compiled)).toEqual({ | ||
| module: "Artist", | ||
| matchedPath: "/artist/:artistID/*", | ||
| isNative: true, | ||
| }) | ||
| }) | ||
|
|
||
| it("falls back to a non-native ReactWebView when nothing matches", () => { | ||
| const compiled = compileEigenRoutes([route("/artwork/:artworkID", "Artwork")]) | ||
| expect(classifyURL(at("/totally/unknown/path"), compiled)).toEqual({ | ||
| module: "ReactWebView", | ||
| matchedPath: null, | ||
| isNative: false, | ||
| }) | ||
| }) | ||
|
|
||
| it("treats webview modules (ModalWebView) as non-native", () => { | ||
| const compiled = compileEigenRoutes([route("/terms", "ModalWebView")]) | ||
| const res = classifyURL(at("/terms"), compiled) | ||
| expect(res.module).toBe("ModalWebView") | ||
| expect(res.isNative).toBe(false) | ||
| }) | ||
| }) |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { execFileSync } from "child_process" | ||
| import { join } from "path" | ||
|
|
||
| // A plain in-process import can't test this: Jest runs under the react-native | ||
| // preset, so the RN module graph is always present and would mask a runtime | ||
| // import in RouteMatcher.ts. We reproduce the real `yarn route-drift` path by | ||
| // loading match.ts in a child tsx process instead. | ||
| const TSX = join(__dirname, "../../../node_modules/.bin/tsx") | ||
| const CHECK_SCRIPT = join(__dirname, "../checkMatchIsolation.ts") | ||
|
|
||
| describe("match.ts isolation", () => { | ||
| it("loads RouteMatcher under tsx without the React Native module graph", () => { | ||
| let output: string | ||
| try { | ||
| output = execFileSync(TSX, [CHECK_SCRIPT], { | ||
| encoding: "utf8", | ||
| stdio: "pipe", | ||
| timeout: 60_000, | ||
| }) | ||
|
github-actions[bot] marked this conversation as resolved.
|
||
| } catch (e: any) { | ||
| throw new Error( | ||
| "match.ts failed to load under tsx without the React Native module graph.\n" + | ||
| "A runtime (non-`import type`) import was likely added to RouteMatcher.ts or match.ts.\n" + | ||
| "Keep their `app/...` imports type-only so scripts/route-drift can load them in isolation.\n\n" + | ||
| `${e.stdout ?? ""}${e.stderr ?? ""}` | ||
| ) | ||
| } | ||
| expect(output).toContain("route-matcher-isolation-ok") | ||
| }) | ||
| }) | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { toMatcher } from "../parseAASA" | ||
|
|
||
| describe("toMatcher", () => { | ||
| it.each([ | ||
| // "/foo/*" excludes /foo/ and anything under it, but NOT the bare /foo | ||
| ["/news/*", "/news/foo", true], | ||
| ["/news/*", "/news/", true], | ||
| ["/news/*", "/news", false], | ||
| ["/news/*", "/newsfoo", false], | ||
| // "/foo*" is a bare prefix match (zero-or-more chars, no slash boundary) | ||
| ["/gender-equality*", "/gender-equality", true], | ||
| ["/gender-equality*", "/gender-equality-2024", true], | ||
| ["/gender-equality*", "/gender", false], | ||
| // exact match | ||
| ["/login", "/login", true], | ||
| ["/login", "/login/x", false], | ||
| ["/login", "/loginx", false], | ||
| // mid-string wildcard -> regex | ||
| ["/a/*/b", "/a/x/b", true], | ||
| ["/a/*/b", "/a/b", false], | ||
| ])("matcher(%p)(%p) === %p", (pattern, path, expected) => { | ||
| expect(toMatcher(pattern)(path)).toEqual(expected) | ||
| }) | ||
| }) |
25 changes: 25 additions & 0 deletions
25
scripts/route-drift/__tests__/parseAndroidManifest.tests.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { matchesRule } from "../parseAndroidManifest" | ||
|
|
||
| describe("matchesRule", () => { | ||
| it.each([ | ||
| // exact path | ||
| [{ kind: "path" as const, value: "/news" }, "/news", true], | ||
| [{ kind: "path" as const, value: "/news" }, "/news/x", false], | ||
| // pathPrefix is substring-prefix (Android semantics — not segment-aware) | ||
| [{ kind: "pathPrefix" as const, value: "/collect" }, "/collect", true], | ||
| [{ kind: "pathPrefix" as const, value: "/collect" }, "/collect/x", true], | ||
| [{ kind: "pathPrefix" as const, value: "/collect" }, "/collection", true], | ||
| [{ kind: "pathPrefix" as const, value: "/collect" }, "/foo", false], | ||
| // pathSuffix | ||
| [{ kind: "pathSuffix" as const, value: ".pdf" }, "/a/b.pdf", true], | ||
| [{ kind: "pathSuffix" as const, value: ".pdf" }, "/a/b.png", false], | ||
| // pathPattern: Android glob (. = any char, * = zero-or-more of preceding) | ||
| [{ kind: "pathPattern" as const, value: "/a/.*" }, "/a/xyz", true], | ||
| [{ kind: "pathPattern" as const, value: "/a/.*" }, "/b", false], | ||
| // advanced-glob escape: `\.` is a literal dot, not "backslash + any char" | ||
| [{ kind: "pathAdvancedPattern" as const, value: "/a\\.pdf" }, "/a.pdf", true], | ||
| [{ kind: "pathAdvancedPattern" as const, value: "/a\\.pdf" }, "/axpdf", false], | ||
| ])("matchesRule(%p, %p) === %p", (rule, pathname, expected) => { | ||
| expect(matchesRule(pathname, rule)).toEqual(expected) | ||
| }) | ||
| }) |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
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?