Repository navigation
feat(web): add /demo page for ChatGPT App review recording - #190
Conversation
The ChatGPT App Directory submission needs a public Demo Recording URL. Add a /demo page (no auth) that hosts a walkthrough video and describes the use cases, following the existing terms/privacy/security page pattern. The video source is config-driven via DEMO_VIDEO_URL: empty renders a "coming soon" placeholder; a YouTube/Loom share URL becomes a responsive iframe; a direct .mp4/.webm renders a native <video>; any other URL links out. The embed URL is reconstructed from a fixed host + extracted id so a crafted value cannot iframe a foreign origin.
|
@claude review |
|
Claude finished @mashkovd's task in 3m 8s —— View job Code Review
Summary: 0 P1 · 0 P2 · 3 P3 The implementation is clean and follows the existing page pattern correctly. Key points checked: Security — Correctness — All classification branches ( Error handling — No errors to handle; the function is pure and the template is compiled at init time via Concurrency — No mutable state; the handler closure captures only immutable P3 nits (non-blocking, see inline comments):
|
|
Claude finished @mashkovd's task in 4m 8s —— View job Code Review
Result: Approved — no P1/P2 findings, 3 P3 nits. SecurityThe security-sensitive design is sound:
P3 nits (non-blocking)
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
P3 — The TestDemo_* handler tests cover only the placeholder and YouTube render paths. Loom, file (native <video>), and link-out kinds each exercise different template branches and are not covered at the handler level. The classifyDemoVideo table already covers classification; adding three more httptest cases (similar to TestDemo_YouTubeEmbed) would give end-to-end confidence that the template renders correctly for every branch.
Also no test for m.youtube.com share URLs — that case is in the switch but absent from the table.
| switch { | ||
| case host == "youtu.be": | ||
| if id := firstPathSegment(path); id != "" { | ||
| return demoVideoYouTube, "https://www.youtube.com/embed/" + id |
There was a problem hiding this comment.
P3 — The extracted video ID is appended to the embed URL without character validation (firstPathSegment can return any non-/ string from the parsed path). Since DEMO_VIDEO_URL is operator-controlled this is low risk, but a guard like validVideoID (accept only [A-Za-z0-9_-]+) would make the origin-isolation guarantee explicit and prevent a misconfigured value from producing a non-functional embed URL silently. The same applies to the Loom ID at line 99.
| type Data struct { | ||
| Title string // <title> text | ||
| NavActive string // "home" | "docs" | "security" | "privacy" | "terms" | "" | ||
| NavActive string // "home" | "docs" | "security" | "privacy" | "terms" | "demo" | "" |
There was a problem hiding this comment.
P3 — "demo" was added to the NavActive comment as a valid value, but the ui_topbar template has no /demo nav item, so NavActive: "demo" produces no visible highlight. If the intent is that demo lives only in the footer (not the top nav), the comment value could be dropped from the list to avoid confusion for future page authors. If a topbar link is desired later, the template entry is missing here.
|
|
||
| // demoData embeds the shared chrome data and adds the resolved video fields. | ||
| // EmbedURL is template.URL so html/template permits it as an iframe/<video> | ||
| // src — it is only ever a URL this package constructed from a recognised host, |
There was a problem hiding this comment.
P3 — The comment says EmbedURL is "never the raw operator-supplied string interpolated into an attribute", but for the demoVideoFile and demoVideoLink cases it is the raw (validated) URL — classifyDemoVideo returns raw directly for both. The security property still holds (http/https scheme + non-empty host were validated, and html/template still HTML-escapes the value in attribute context), but the doc comment is misleading.
| // src — it is only ever a URL this package constructed from a recognised host, | |
| // demoData embeds the shared chrome data and adds the resolved video fields. | |
| // EmbedURL is template.URL so html/template permits it as an iframe/<video> | |
| // src or anchor href. For youtube/loom it is a URL reconstructed from a fixed | |
| // host + extracted id; for file/link it is the validated operator URL (http/https | |
| // with non-empty host). In all cases html/template HTML-escapes it in attribute | |
| // context. |
| data := demoData{ | ||
| Data: ui.Data{ | ||
| Title: "MCTL Telegram Demo", | ||
| NavActive: "demo", |
There was a problem hiding this comment.
P3 — NavActive: "demo" is set, and the ui.Data comment was updated to include "demo" as a valid value, but the topbar template (ui_topbar) has no /demo nav item and no {{if eq .NavActive "demo"}} active-state check. The footer gained the link (correct), but the topbar didn't. As a result this field has no effect for the demo page.
Either add a /demo link + active-check to the topbar (keeping parity with other pages), or leave it out of the nav intentionally and set NavActive: "" here to avoid implying the feature works.
| if !strings.Contains(body, "<iframe") { | ||
| t.Error("youtube page should render an iframe") | ||
| } | ||
| } |
There was a problem hiding this comment.
P3 — Handler-level render tests cover placeholder and YouTube embed but not the "file" (<video> element) or "link" (anchor) branches. classifyDemoVideo unit tests cover the classification logic, so the risk is low, but a render test for each remaining branch would lock in the HTML shape. For example:
func TestDemo_FileEmbed(t *testing.T) {
w := httptest.NewRecorder()
Demo("https://tg.mctl.ai", "https://cdn.example.com/demo.mp4").ServeHTTP(w, httptest.NewRequest("GET", "/demo", nil))
if w.Code != 200 {
t.Fatalf("expected 200, got %d", w.Code)
}
body := w.Body.String()
if !strings.Contains(body, "<video") {
t.Error("file page should render a <video> element")
}
}
There was a problem hiding this comment.
No P1/P2 findings (3 P3). Good to merge.
The security-sensitive parts are well-handled: embed URLs for YouTube/Loom are reconstructed from a fixed-prefix + extracted ID (no cross-origin iframe injection possible), http(s) scheme + non-empty host are validated before any template.URL cast, and html/template contextual escaping covers the file/link render paths. The config-driven approach with a clean placeholder fallback is solid.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
/demopage on tg.mctl.ai for the ChatGPT App Directory Demo Recording URL field (already set tohttps://tg.mctl.ai/demoin the submission form).terms.html/privacy.html/security.htmlpattern (Go template through the sharedinternal/uichrome), not a new stack.DEMO_VIDEO_URL:.mp4/.webm→ native<video controls>/demolink.Notes
DEMO_VIDEO_URLis a URL (containshttps://). When wiring the real video, hand-edit gitopsvalues.yaml—mctl_deploy_servicestrips colon-bearing env vars.Test plan
go build ./... && go vet ./...go test ./internal/web/... ./internal/config/...(newclassifyDemoVideotable + placeholder/youtube render tests)GET /demo→ 200 (placeholder),/terms/privacystill 200🤖 Generated with Claude Code