Repository navigation
feat(mobile): read and comment on pages from the phone #7367
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
Changes from all commits
8aca833
7ccb02e
7ab8d92
b6d0156
6dfd534
695925f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { OrganizationSwitchSheet } from "@/screens/(authenticated)/(home)/organizations/switch"; | ||
|
|
||
| export default OrganizationSwitchSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { AllCommentsSheet } from "@/screens/(authenticated)/pages/[slug]/comments"; | ||
|
|
||
| export default AllCommentsSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { ComposeCommentSheet } from "@/screens/(authenticated)/pages/[slug]/compose"; | ||
|
|
||
| export default ComposeCommentSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { PageDetailScreen } from "@/screens/(authenticated)/pages/[slug]"; | ||
|
|
||
| export default PageDetailScreen; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { PagePreviewSheet } from "@/screens/(authenticated)/pages/[slug]/preview"; | ||
|
|
||
| export default PagePreviewSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { QuickFeedbackSheet } from "@/screens/(authenticated)/pages/[slug]/quick"; | ||
|
|
||
| export default QuickFeedbackSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { PageShareSheet } from "@/screens/(authenticated)/pages/[slug]/share"; | ||
|
|
||
| export default PageShareSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { CommentThreadSheet } from "@/screens/(authenticated)/pages/[slug]/thread"; | ||
|
|
||
| export default CommentThreadSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { PagesFilterSheet } from "@/screens/(authenticated)/pages/filter"; | ||
|
|
||
| export default PagesFilterSheet; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| import { PagesScreen } from "@/screens/(authenticated)/pages"; | ||
|
|
||
| export default PagesScreen; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { useOpenLink } from "./useOpenLink"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { useRouter } from "expo-router"; | ||
| import { useCallback } from "react"; | ||
| import { env } from "@/lib/env"; | ||
| import { openUrl } from "@/lib/open-url"; | ||
| import { pageSlugFromUrl } from "@/lib/page-links"; | ||
|
|
||
| export function useOpenLink(): (url: string) => void { | ||
| const router = useRouter(); | ||
|
|
||
| return useCallback( | ||
| (url: string) => { | ||
| const slug = pageSlugFromUrl(url, env.EXPO_PUBLIC_WEB_URL); | ||
| if (slug === null) { | ||
| openUrl(url); | ||
| return; | ||
| } | ||
| router.push({ | ||
| pathname: "/(authenticated)/pages/[slug]/preview", | ||
| params: { slug }, | ||
| }); | ||
| }, | ||
| [router], | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { pageSlugFromUrl } from "./pageLinks"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { pageSlugFromUrl } from "./pageLinks"; | ||
|
|
||
| const WEB = "https://app.superset.sh"; | ||
|
|
||
| describe("pageSlugFromUrl", () => { | ||
| test("reads the slug from a page link", () => { | ||
| expect(pageSlugFromUrl(`${WEB}/page/one-plugin-surface-q7x`, WEB)).toBe( | ||
| "one-plugin-surface-q7x", | ||
| ); | ||
| }); | ||
|
|
||
| test("ignores a trailing slash, query and fragment", () => { | ||
| expect(pageSlugFromUrl(`${WEB}/page/deploy-notes/`, WEB)).toBe( | ||
| "deploy-notes", | ||
| ); | ||
| expect(pageSlugFromUrl(`${WEB}/page/deploy-notes?v=3#top`, WEB)).toBe( | ||
| "deploy-notes", | ||
| ); | ||
| }); | ||
|
|
||
| test("tolerates a configured web url with a trailing slash", () => { | ||
| expect(pageSlugFromUrl(`${WEB}/page/deploy-notes`, `${WEB}/`)).toBe( | ||
| "deploy-notes", | ||
| ); | ||
| }); | ||
|
|
||
| test("refuses another origin, however similar", () => { | ||
| expect(pageSlugFromUrl("https://superset.sh/page/deploy-notes", WEB)).toBe( | ||
| null, | ||
| ); | ||
| expect( | ||
| pageSlugFromUrl("https://app.superset.sh.evil.example/page/x", WEB), | ||
| ).toBe(null); | ||
| expect(pageSlugFromUrl(`http://app.superset.sh/page/x`, WEB)).toBe(null); | ||
| }); | ||
|
|
||
| test("refuses paths that are not a single page", () => { | ||
| expect(pageSlugFromUrl(`${WEB}/pages/deploy-notes`, WEB)).toBe(null); | ||
| expect(pageSlugFromUrl(`${WEB}/page`, WEB)).toBe(null); | ||
| expect(pageSlugFromUrl(`${WEB}/page/`, WEB)).toBe(null); | ||
| expect(pageSlugFromUrl(`${WEB}/page/deploy-notes/versions`, WEB)).toBe( | ||
| null, | ||
| ); | ||
| }); | ||
|
|
||
| test("refuses anything that is not a url", () => { | ||
| expect(pageSlugFromUrl("not a url", WEB)).toBe(null); | ||
| expect(pageSlugFromUrl("", WEB)).toBe(null); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| export function pageSlugFromUrl(url: string, webUrl: string): string | null { | ||
| let target: URL; | ||
| let web: URL; | ||
| try { | ||
| target = new URL(url); | ||
| web = new URL(webUrl); | ||
| } catch { | ||
| return null; | ||
| } | ||
| if (target.origin !== web.origin) return null; | ||
|
|
||
| const segments = target.pathname.split("/").filter(Boolean); | ||
| if (segments.length !== 2 || segments[0] !== "page") return null; | ||
| return segments[1] ?? null; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: When a page slug contains URL-encoded characters, Prompt for AI agents |
||
| } | ||
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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add
...glassHeaderOptionsto theorganizations/indexform sheet.The mobile routing contract requires this spread for
formSheetroutes. Without it, the native header lacks the required glass treatment and close control.🤖 Prompt for AI Agents