feat: launch-week feedback banner above the top bar - #168
Conversation
Full-width, non-dismissible banner linking 'Leave feedback or ask for features here' to discussion #163. Rendered inside the h-svh flex column so panes absorb its height. Included in the prerendered dist; SEO check extended to assert banner copy + link in built index.html.
…ant grep The launch-week banner links to github.com//discussions/163, which the zero-tolerance 'flatfile' remnant pattern matches. Filter only that exact URL out of the match set; every other hit still fails the shim job.
![obvious-autobuild-staging[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
There was a problem hiding this comment.
Obvious Code Review
Verdict: COMMENT (1 High, 1 Medium, 1 Suggestion)
High
.github/workflows/ci.yml:89— Remnant-grep exemption filters whole lines; the main JS bundle is one 107KB line containing the sanctioned URL, so the entire application chunk is exempt from the zero-telemetry gate.
Medium
app/src/components/FeedbackBanner.tsx:17—truncateclips the feedback link below ~525px viewports; mobile users get a permanently ellipsized CTA in a non-dismissible banner.
Suggestions
app/src/components/FeedbackBanner.tsx:8— the discussion URL is now hardcoded in four places (component, test, verify-seo.sh, ci.yml); consider a single source or cross-referencing comment to ease the post-launch teardown.
| # GitHub discussions (contains "FlatFilers") — the sanctioned | ||
| # URL is exempt; everything else stays zero-tolerance. | ||
| if grep -riE 'segment|gtag|linkedin|carbonads|chikita|flatfile|typekit|putObject' "${targets[@]}" \ | ||
| | grep -viF 'github.com/FlatFilers/csvjson-app/discussions/163'; then |
There was a problem hiding this comment.
High · security — The remnant-grep exemption drops whole lines containing the sanctioned URL, but the main bundle app/dist/assets/index-UreU1wdh.js is minified to a handful of lines and the URL sits on a single 107,119-byte line holding essentially the whole app chunk. grep -viF exempts that entire line, so any future segment/gtag/carbonads/flatfile remnant anywhere in the main chunk passes CI silently — spec criterion 8's zero-tolerance gate is fail-open for the largest shipped artifact (verified by simulation: the exemption is the only reason the job passes on this tree). Strip the URL substring from matched lines and re-grep instead of filtering lines, e.g. grep -riE '…' "${targets[@]}" | sed 's#github[.]com/FlatFilers/csvjson-app/discussions/163##g' | grep . — the sed removes only the URL occurrence; every other pattern word on that 107KB line still fails the build.
There was a problem hiding this comment.
Replying to this comment
Fixed in f086136 — the pipeline now strips the sanctioned URL from matched lines and re-greps with the same pattern. One correction to the suggested snippet: ending withgrep .would false-positive, because stripping the URL leaves non-empty residue (var u="https://";) on lines whose only hit was the sanctioned URL. The re-grep re-checks the remnant pattern itself, so only lines that still contain a real pattern after stripping fail. Verified: a synthetic minified line containing both the URL andgtagis caught; a sanctioned-URL-only line passes; the real tree (including the fresh bundle) passes.
| data-testid="feedback-banner" | ||
| className="flex flex-shrink-0 items-center justify-center border-b border-border bg-muted px-4 py-1.5 text-sm text-muted-foreground" | ||
| > | ||
| <p className="truncate"> |
There was a problem hiding this comment.
Medium · reliability — The <p> carries truncate (nowrap + ellipsis) over the full 74-char phrase (~525px at text-sm). On a 360–375px phone (~343px available after px-4) the tail of the linked phrase — the banner's sole call to action — is ellipsized ("…Leave feedback or a…"), and the banner is non-dismissible by design, so mobile users see a permanently clipped CTA for launch week. Keep the link intact on small screens, e.g. hide the lead-in below sm (<span className="hidden sm:inline">Enjoy a cleaner, simpler CSVJSON.{" "}</span>) so the link fits, or apply the truncation to the lead-in span rather than the whole paragraph.
There was a problem hiding this comment.
Replying to this comment
Fixed in f086136 — droppedtruncateand centered the paragraph, so it wraps instead of ellipsizing. I went with wrap rather than hiding the lead-in belowsmbecause the launch-week spec mandates the exact full copy, and hiding the lead-in on phones would violate that; a two-line banner on the narrowest viewports is acceptable since the flex panes absorb the height.
- ci: strip the sanctioned discussions URL from matched lines and re-grep with the same pattern instead of dropping whole lines — a line-filter exempted the entire 107KB minified bundle line from the zero-telemetry gate. Verified: same-line gtag remnant is caught, a sanctioned-URL-only line passes. - FeedbackBanner: drop truncate; the CTA no longer ellipsizes on 360-375px phones — the paragraph wraps and stays fully readable. - Cross-reference FEEDBACK_DISCUSSION_URL from verify-seo.sh so the post-launch teardown has one place to look.
|
There was a problem hiding this comment.
Obvious Code Review
Verdict: COMMENT — Pass 2 clean.
No findings — clean pass.
All three Pass 1 findings resolved and verified at head f086136:
- High (ci.yml remnant-grep exemption) — the pipeline now strips the sanctioned URL substring from matched lines and re-greps. Verified by simulation: current tree passes, an injected
gtagon the 107KB bundle line fails the gate, and a remnant sharing a line with the sanctioned URL (the original failure mode) is caught. - Medium (banner CTA truncation) —
truncatereplaced withtext-center; the phrase wraps on narrow viewports and the rebuilt prerender still carries the banner copy and link. - Suggestion — canonical-source comments added to verify-seo.sh and ci.yml.
|
1 participant
Why
Launch week needs a visible channel for feedback. A thin, non-dismissible banner above the top bar routes users straight to discussion #163 without displacing the converter, and comes down as soon as the team calls it.
What
FeedbackBanner(new component): full-width bar at the very top, above the TopBar. Copy: "Enjoy a cleaner, simpler CSVJSON. Leave feedback or ask for features here." — the whole phrase links to Welcome to the new CSVJSON app! #163,target=_blank,rel="noopener noreferrer".h-svhflex column, so the flex panes absorb the banner's height instead of the page overflowing. No changes to the below-768px stacked layout — on mobile it simply sits above the top bar. Options bar never clips because SplitPane usesflex-1 min-h-0.bg-muted,border-border,muted-foreground/foreground),py-1.5 text-smto stay thinner than the TopBar; visible focus ring on the link (focus-visible:outline-ring). Dark and light themes both covered by tokens. No dismiss button — v1 is non-dismissible by design.app/dist(prerender includes the banner) for the CI dist-freshness gate..github/scripts/verify-seo.shto assert the banner copy and discussion link exist in the builtindex.html.fix(ci): the shim job's zero-tolerance "flatfile" remnant grep now matches the mandated discussion URL (the app's own GitHub org name). The check filters exactly that sanctioned URL out of the match set; verified a real remnant (gtag) is still caught.How to Review
app/src/components/FeedbackBanner.tsx+ its test (asserts exact copy, href, target, rel, and DOM order above the TopBar).app/src/App.tsx— two-line mount..github/workflows/ci.yml— one narrow exemption to the remnant grep, required because the sanctioned URL contains "flatfile".Test Evidence
Local: lint, typecheck, 94/94 vitest tests, production build + prerender,
verify-seo.sh, and the modified remnant grep (positive and negative) all green; banner copy and link confirmed in builtindex.html.🔗 Obvious Project · 🧵 Obvious Thread