feat: improve tools panel flow and active result context#23
feat: improve tools panel flow and active result context#23rohan-patnaik merged 6 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughUpdates UI theme and spacing, adds a runtime environment indicator and header/layout changes, enhances the tools page with contextual job/result tracking and download actions, adds a Capacity Logic page and global limits, refactors Donate/UPI flow, wires Clerk publishable key and proxy auth checks, and introduces several stability and storage-tolerance tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Web UI (tools/page)
participant API as Server/Convex
participant Storage as Artifact/Download
User->>UI: Select tool / create job
UI->>API: Create job request (toolId, inputs)
API-->>UI: Job queued (jobId)
Note right of API: Job executes asynchronously
API->>Storage: Store artifact/result (on completion)
API-->>UI: Job status update (succeeded/done, finishedAt, artifact metadata)
UI->>UI: derive contextResult/contextResultSummary
User->>UI: Click Download contextual result
UI->>Storage: Download artifact (signed URL or proxy)
Storage-->>User: Return file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/app/tools/page.tsx`:
- Around line 806-843: The contextResult compute currently filters out any
non-"succeeded" jobs (see useMemo block and the normalizedStatus check that
returns undefined unless normalizedStatus === "succeeded"), making the status
badge branches for "failed" and "running" unreachable; fix by either (A)
allowing in-progress/failed context by removing the normalizedStatus !==
"succeeded" guard so contextResult is returned for matchingJob/output regardless
of status (and keep normalizedStatus in the returned object), or (B) if the
intent is succeeded-only, simplify the UI by removing the "failed"/"running"
branches and always render "Succeeded" for contextResult — update the useMemo
logic around matchingJob, normalizedStatus, and the UI badge accordingly (refer
to contextResult, normalizedStatus, matchingJob, and the status badge rendering
code).
🧹 Nitpick comments (2)
apps/web/src/app/tools/page.tsx (2)
806-843:contextResultduplicates logic already inJobCard.The input-size extraction (Lines 821–824), compression-result derivation (Lines 825–830), and savings formatting are repeated from
JobCard. Consider extracting a shared helper (e.g.,deriveJobSummary(job)) to keep these in sync and reduce duplication.Also applies to: 846-879
1279-1325: Inline context result block — minor accessibility note.The download button and summary strip appear/disappear based on
contextResult. Screen reader users won't be notified of the new content. Consider wrapping this block in an element withrole="status"oraria-live="polite"(similar to the status message at Line 1329) so assistive technology announces the result availability.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/web/src/app/providers.tsx`:
- Around line 28-29: The ClerkProvider is being passed publishableKey even when
clerkPublishableKey may be undefined; update the component usage (ClerkProvider)
to either remove the publishableKey prop entirely so Clerk auto-detects
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY, or conditionally pass it only when defined
(use a conditional spread or ternary around clerkPublishableKey) so you don't
pass publishableKey={undefined}; locate the ClerkProvider declaration in
providers.tsx and change the prop handling accordingly.
In `@apps/web/src/components/DonateBookmark.tsx`:
- Around line 55-56: In DonateBookmark, qrUrl is always set to a truthy value
because it falls back to defaultUpiQrImage, which prevents generatedQrUrl from
ever being used; change qrUrl to only reflect the explicit config (const qrUrl =
configuredQrUrl) and update resolvedQrUrl to prefer generated QR when no
explicit URL is provided (resolvedQrUrl = qrUrl || generatedQrUrl ||
defaultUpiQrImage), and adjust the QR generation useEffect to only run when
there is no configuredQrUrl (useEffect dependency on configuredQrUrl and guard
so it generates only when configuredQrUrl is empty) so dynamic generation is
used when appropriate and the static fallback only applies last.
In `@apps/web/src/components/SiteHeader.tsx`:
- Around line 60-90: Two sibling <nav> landmarks in SiteHeader.tsx both use
aria-label="Primary", causing duplicate navigation announcements; update the
aria-labels on the two <nav> elements to be unique (e.g., change the desktop nav
— the element with className "hidden min-w-0 flex-1 items-center justify-end
gap-2 text-sm lg:flex lg:flex-wrap" — to "Primary navigation" and the mobile nav
— the element with className "mobile-scroll-row flex min-w-0 flex-1 items-center
gap-2 overflow-x-auto pb-1 text-sm" — to "Primary navigation (mobile)"), or
alternatively refactor to render a single <nav> responsively; make the change in
the two <nav> elements in SiteHeader.tsx so screen readers see distinct
landmarks.
🧹 Nitpick comments (5)
apps/web/src/components/RuntimeEnvironmentIndicator.tsx (1)
7-8: Regex accepts octets > 255 (e.g.,10.999.999.999).
\d{1,3}matches 0–999, not just valid octets 0–255. In practice this is harmless since browsers won't produce invalid IP hostnames, but worth noting for correctness.apps/web/src/app/globals.css (1)
247-253: Hidden scrollbar may reduce scroll discoverability.The
.mobile-scroll-rowhides both the standard scrollbar (scrollbar-width: none) and WebKit scrollbar. For horizontally scrollable content (the "Quick switch" tool row intools/page.tsx), users may not realize the row is scrollable. Consider adding a visual affordance like a fade/gradient at the edges or keeping a thin scrollbar visible.apps/web/src/app/tools/page.tsx (2)
783-810:toolCatalogreferencesselectToolbefore its declaration — safe at runtime but fragile.
toolCatalogis defined at Line 783 and callsselectToolin itsonClickhandlers, butselectToolis declared at Line 909. This works because the click handlers are closures that only resolveselectToolat invocation time (after the component body has fully executed), so there's no TDZ error. However, this ordering is fragile — if someone refactorsselectToolinto a different scope or an early-invoked callback, it would break silently. Consider movingtoolCatalogbelowselectTool, or extracting it as a local component.
1149-1155: Tool catalog is rendered in two DOM locations.
toolCatalogis rendered both inside a<details>(mobile,lg:hidden) and a<div>(desktop,hidden lg:block). This means the full catalog DOM exists twice. This is a standard responsive pattern, but with 26 tools × 2, it's a non-trivial amount of duplicated DOM. If performance on low-end devices becomes a concern, consider conditionally rendering based on a media query hook instead.apps/web/src/components/DonateBookmark.tsx (1)
37-40: Hardcoded personal UPI ID in source code.
defaultUpiIdcontains a real UPI identifier (rohanpatnaik1997-1@okhdfcbank). While this is intentional for the donation use case and overridable viaNEXT_PUBLIC_DONATE_UPI_ID, be aware this is committed to the public repo. For a personal open-source project this is likely acceptable, but consider documenting that self-hosters should override these defaults.
| <ClerkProvider | ||
| publishableKey={clerkPublishableKey} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ClerkProvider publishableKey prop undefined fallback environment variable
💡 Result:
In Next.js, you typically don’t need to pass publishableKey to <ClerkProvider /> at all—Clerk will read it from the environment as long as you set:
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY=pk_(test|live)_...(frontend) (clerk.com)
So if you’re seeing publishableKey as undefined, the usual fixes are:
- Set the correct env var name
- Use
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY(not justCLERK_PUBLISHABLE_KEY) for anything that runs in the browser. (clerk.com)
- Restart the dev server after editing
.env*
- Next.js won’t pick up new env vars reliably without a restart (stop + start).
- Don’t pass an undefined prop
- This is fine (recommended) if your env var is set:
Clerk’s Next.js package explicitly documents relying on
<ClerkProvider>{children}</ClerkProvider>
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEYfor the Provider. (npmjs.com)
- If deployed, ensure the env var exists in the hosting provider
- The common “Missing publishableKey” runtime error happens when the deployed environment doesn’t actually have
NEXT_PUBLIC_CLERK_PUBLISHABLE_KEYavailable. (stackoverflow.com)
If you tell me your framework target (@clerk/nextjs vs @clerk/clerk-react, App Router vs Pages Router, and where you set env vars—local vs Vercel/Netlify/etc.), I can narrow it down to the exact cause.
Citations:
- 1: https://clerk.com/docs/guides/development/clerk-environment-variables?utm_source=openai
- 2: https://clerk.com/docs/guides/development/clerk-environment-variables?utm_source=openai
- 3: https://www.npmjs.com/package/%40clerk/nextjs/v/4.27.2-canary.ve61df05?utm_source=openai
- 4: https://stackoverflow.com/questions/76248386/clerk-fails-to-get-the-publishable-key-from-the-environment-variables-in-next-js?utm_source=openai
🏁 Script executed:
head -40 apps/web/src/app/providers.tsxRepository: rohan-patnaik/ZenPDF
Length of output: 1438
Remove or conditionally pass publishableKey to align with Clerk's documented best practice.
Clerk explicitly recommends omitting the publishableKey prop entirely and letting it auto-detect from NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY. Passing publishableKey={undefined} in development is not a documented fallback pattern. Either omit the prop: <ClerkProvider> or conditionally pass it only when defined:
<ClerkProvider
publishableKey={clerkPublishableKey}
signInUrl="/sign-in"
signUpUrl="/sign-up"should be:
<ClerkProvider
{...(clerkPublishableKey && { publishableKey: clerkPublishableKey })}
signInUrl="/sign-in"
signUpUrl="/sign-up"Or simply remove the prop and trust Clerk's environment variable detection.
🤖 Prompt for AI Agents
In `@apps/web/src/app/providers.tsx` around lines 28 - 29, The ClerkProvider is
being passed publishableKey even when clerkPublishableKey may be undefined;
update the component usage (ClerkProvider) to either remove the publishableKey
prop entirely so Clerk auto-detects NEXT_PUBLIC_CLERK_PUBLISHABLE_KEY, or
conditionally pass it only when defined (use a conditional spread or ternary
around clerkPublishableKey) so you don't pass publishableKey={undefined}; locate
the ClerkProvider declaration in providers.tsx and change the prop handling
accordingly.
| <nav | ||
| className="hidden min-w-0 flex-1 items-center justify-end gap-2 text-sm lg:flex lg:flex-wrap" | ||
| aria-label="Primary" | ||
| > | ||
| Usage & Capacity | ||
| </Link> | ||
| <ThemeToggle /> | ||
| <SignedOut> | ||
| <SignInButton mode="modal"> | ||
| <button className="paper-button" type="button"> | ||
| Sign in with Google | ||
| </button> | ||
| </SignInButton> | ||
| </SignedOut> | ||
| <SignedIn> | ||
| <UserButton afterSignOutUrl="/" /> | ||
| </SignedIn> | ||
| </nav> | ||
| {navLinks} | ||
| <RuntimeEnvironmentIndicator /> | ||
| <SignedOut> | ||
| <SignInButton mode="modal"> | ||
| <button className="paper-button hidden lg:inline-flex" type="button"> | ||
| Sign in with Google | ||
| </button> | ||
| </SignInButton> | ||
| </SignedOut> | ||
| <div className="ml-1 flex shrink-0 items-center gap-2"> | ||
| <ThemeToggle /> | ||
| <SignedIn> | ||
| <UserButton afterSignOutUrl="/" /> | ||
| </SignedIn> | ||
| </div> | ||
| </nav> | ||
| </div> | ||
|
|
||
| <div className="mt-2 flex items-center gap-2 lg:hidden"> | ||
| <nav | ||
| className="mobile-scroll-row flex min-w-0 flex-1 items-center gap-2 overflow-x-auto pb-1 text-sm" | ||
| aria-label="Primary" | ||
| > | ||
| {navLinks} | ||
| </nav> | ||
| <RuntimeEnvironmentIndicator compact /> | ||
| </div> |
There was a problem hiding this comment.
Duplicate aria-label="Primary" on two sibling <nav> landmarks.
Both the desktop nav (line 60) and mobile nav (line 83) carry aria-label="Primary". CSS responsive hiding (hidden/lg:hidden) doesn't remove elements from the accessibility tree, so screen readers will announce two "Primary" navigation landmarks. Differentiate them — e.g., "Primary navigation" and "Primary navigation (mobile)" — or render only one <nav> and adjust its contents responsively.
Proposed fix
<nav
- className="hidden min-w-0 flex-1 items-center justify-end gap-2 text-sm lg:flex lg:flex-wrap"
- aria-label="Primary"
+ className="hidden min-w-0 flex-1 items-center justify-end gap-2 text-sm lg:flex lg:flex-wrap"
+ aria-label="Primary navigation"
> <nav
- className="mobile-scroll-row flex min-w-0 flex-1 items-center gap-2 overflow-x-auto pb-1 text-sm"
- aria-label="Primary"
+ className="mobile-scroll-row flex min-w-0 flex-1 items-center gap-2 overflow-x-auto pb-1 text-sm"
+ aria-label="Primary navigation (mobile)"
>🤖 Prompt for AI Agents
In `@apps/web/src/components/SiteHeader.tsx` around lines 60 - 90, Two sibling
<nav> landmarks in SiteHeader.tsx both use aria-label="Primary", causing
duplicate navigation announcements; update the aria-labels on the two <nav>
elements to be unique (e.g., change the desktop nav — the element with className
"hidden min-w-0 flex-1 items-center justify-end gap-2 text-sm lg:flex
lg:flex-wrap" — to "Primary navigation" and the mobile nav — the element with
className "mobile-scroll-row flex min-w-0 flex-1 items-center gap-2
overflow-x-auto pb-1 text-sm" — to "Primary navigation (mobile)"), or
alternatively refactor to render a single <nav> responsively; make the change in
the two <nav> elements in SiteHeader.tsx so screen readers see distinct
landmarks.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/components/DonateBookmark.tsx`:
- Around line 73-78: The current useMemo-based isProbablyMobileDevice runs
during SSR (returning false) causing hydration mismatches; replace the useMemo
constant with a function named isProbablyMobileDevice() that checks typeof
window !== "undefined" and tests navigator.userAgent at call time, and update
launchUpiApp to call isProbablyMobileDevice() inside the click handler instead
of reading a memoized value; remove the useMemo import/usage so the device check
never runs during SSR and only executes on the client when the handler is
invoked.
In `@apps/web/src/components/SiteHeader.tsx`:
- Line 56: The <UserButton> component is passing the deprecated afterSignOutUrl
prop; remove the prop from <UserButton> (symbol: UserButton) since
afterSignOutUrl="/" is redundant, or if you need a global custom redirect, move
the setting into the ClerkProvider configuration in your providers.tsx (symbol:
ClerkProvider / providers.tsx) by setting the afterSignOutUrl there instead of
on UserButton.
🧹 Nitpick comments (4)
apps/web/src/components/DonateBookmark.tsx (2)
378-378: Redundant fallback:upiIdalready incorporatesdefaultUpiId.
upiIdis derived viapickFirstNonEmptyValue(..., defaultUpiId)followed bynormalizeUpiId, so it will always equal the normalized default when the env var is unset. The|| defaultUpiIdhere would only kick in ifnormalizeUpiIdreturned"", which it can't given a non-empty input. The fallback is dead code and slightly misleading.- {upiId || defaultUpiId} + {upiId}
156-169: Consider extracting the repeated timeout-cleanup logic.The same clear-timeout-and-nullify pattern appears in
handleOpen,handleClose, and the unmount effect. A small helper would reduce duplication:Example
+ const clearUpiLaunchTimeout = useCallback(() => { + if (upiLaunchTimeoutRef.current !== null) { + window.clearTimeout(upiLaunchTimeoutRef.current); + upiLaunchTimeoutRef.current = null; + } + }, []);Then replace each inline block with
clearUpiLaunchTimeout().Also applies to: 171-181
apps/web/src/app/tools/page.tsx (2)
834-872: Past review concern is properly addressed — status badge is now always "Succeeded".The
normalizedStatusfilter (Line 846) still computes"failed"/"running"branches (Lines 843-845) that can never survive the guard. Consider simplifying:♻️ Optional simplification to remove dead normalization branches
- const normalizedStatus = - matchingJob?.status === "succeeded" || matchingJob?.status === "done" - ? "succeeded" - : matchingJob?.status === "failed" - ? "failed" - : "running"; - if (!matchingJob || !output || normalizedStatus !== "succeeded") { + const isSucceeded = + matchingJob?.status === "succeeded" || matchingJob?.status === "done"; + if (!matchingJob || !output || !isSucceeded) { return undefined; }Then drop
statusfrom the returned object (the UI already hardcodes "Succeeded").
1174-1193: Quick-switch horizontal row for mobile — minor accessibility enhancement possible.The row has
aria-pressedon each button (good), but the scrollable container itself has no accessible label. A screen reader user encountering this section won't know its purpose without visual context.♻️ Suggested enhancement
- <div className="mt-4 lg:hidden"> - <p className="field-label">Quick switch</p> - <div className="mobile-scroll-row mt-2 flex gap-2 overflow-x-auto pb-1"> + <div className="mt-4 lg:hidden"> + <p className="field-label" id="quick-switch-label">Quick switch</p> + <div role="toolbar" aria-labelledby="quick-switch-label" className="mobile-scroll-row mt-2 flex gap-2 overflow-x-auto pb-1">
| const isProbablyMobileDevice = useMemo(() => { | ||
| if (typeof window === "undefined") { | ||
| return false; | ||
| } | ||
| return /Android|iPhone|iPad|iPod|Mobile/i.test(window.navigator.userAgent); | ||
| }, []); |
There was a problem hiding this comment.
Hydration mismatch on mobile devices.
useMemo with [] runs during SSR (where window is undefined → false) and again on the client. On a mobile device the client computation returns true, producing a React hydration mismatch warning. Since the value is only needed inside launchUpiApp (a click handler), you can avoid memoizing it entirely and just evaluate it at call time:
Proposed fix
- const isProbablyMobileDevice = useMemo(() => {
- if (typeof window === "undefined") {
- return false;
- }
- return /Android|iPhone|iPad|iPod|Mobile/i.test(window.navigator.userAgent);
- }, []);
+ const isProbablyMobileDevice = useCallback(() => {
+ if (typeof window === "undefined") {
+ return false;
+ }
+ return /Android|iPhone|iPad|iPod|Mobile/i.test(window.navigator.userAgent);
+ }, []);Then in launchUpiApp, call isProbablyMobileDevice() instead of reading it as a value. This avoids SSR computation entirely and removes it from the hook's dependency array.
🤖 Prompt for AI Agents
In `@apps/web/src/components/DonateBookmark.tsx` around lines 73 - 78, The current
useMemo-based isProbablyMobileDevice runs during SSR (returning false) causing
hydration mismatches; replace the useMemo constant with a function named
isProbablyMobileDevice() that checks typeof window !== "undefined" and tests
navigator.userAgent at call time, and update launchUpiApp to call
isProbablyMobileDevice() inside the click handler instead of reading a memoized
value; remove the useMemo import/usage so the device check never runs during SSR
and only executes on the client when the handler is invoked.
| </SignInButton> | ||
| </SignedOut> | ||
| <SignedIn> | ||
| <UserButton afterSignOutUrl="/" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@clerk/nextjs v6.36.10 UserButton afterSignOutUrl prop documentation
💡 Result:
In @clerk/nextjs@6.36.10, the <UserButton /> prop afterSignOutUrl?: string is documented, but it is deprecated:
afterSignOutUrl(deprecated): “The full URL or path to navigate to after a successful sign-out.” Clerk’s docs say to moveafterSignOutUrlto<ClerkProvider />instead. [1][2]
Related note from Clerk’s Core 2 / Next.js upgrade guide:
- The default “after sign-out” redirect is now
/, so explicitly passingafterSignOutUrl="/"is no longer necessary in Core 2. [3]
Docs pages: the <UserButton /> component reference lists afterSignOutUrl under Properties with the deprecation note. [1][2]
Sources
[1] Clerk Docs – <UserButton /> component (Next.js reference)
[2] Clerk Docs – <UserButton /> component (Next.js components page)
[3] Clerk Docs – Upgrading @clerk/nextjs to Core 2 (Next.js)
🏁 Script executed:
find . -name "package.json" -o -name "package-lock.json" -o -name "pnpm-lock.yaml" -o -name "yarn.lock" | head -5Repository: rohan-patnaik/ZenPDF
Length of output: 116
🏁 Script executed:
grep -r "@clerk/nextjs" --include="package.json" --include="*.ts" --include="*.tsx" | head -10Repository: rohan-patnaik/ZenPDF
Length of output: 794
🏁 Script executed:
sed -n '50,80p' apps/web/src/components/SiteHeader.tsxRepository: rohan-patnaik/ZenPDF
Length of output: 1149
🏁 Script executed:
cat -n apps/web/src/app/providers.tsxRepository: rohan-patnaik/ZenPDF
Length of output: 4041
Remove deprecated afterSignOutUrl prop or migrate to <ClerkProvider>.
The afterSignOutUrl prop on <UserButton> is deprecated as of @clerk/nextjs v6.36.10. Clerk recommends moving this setting to <ClerkProvider> instead. Additionally, afterSignOutUrl="/" is now redundant since the default after sign-out redirect is /. Either remove the prop entirely or configure it at the provider level in providers.tsx if a custom redirect is needed globally.
🤖 Prompt for AI Agents
In `@apps/web/src/components/SiteHeader.tsx` at line 56, The <UserButton>
component is passing the deprecated afterSignOutUrl prop; remove the prop from
<UserButton> (symbol: UserButton) since afterSignOutUrl="/" is redundant, or if
you need a global custom redirect, move the setting into the ClerkProvider
configuration in your providers.tsx (symbol: ClerkProvider / providers.tsx) by
setting the afterSignOutUrl there instead of on UserButton.
1 participant
Summary
DownloadbesideQueue jobSucceeded)Output X from Y)Saved Z (P%))Validation
cd apps/web && npm run lintcd apps/web && npx tsc --noEmitcd apps/web && npm testcd apps/web && npm run buildCodeRabbit
coderabbit review --plain --type uncommitted --base maintry after ~3m26s)Scope
Summary by CodeRabbit
New Features
Style
Bug Fixes / Improvements
Chores