fix: metastate-requested-changes #348
Conversation
WalkthroughAdjusted styling on three login screens to center and de-emphasize informational text (smaller font size, lighter color, centered alignment) and removed a decorative logo; no functional, data-flow, or exported API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing touches
🧪 Generate unit tests
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (1)
171-179: Verify Group Charter–specific login screen tasks
- ✅ Single W3DS logo present and centered
- ✅ “60 seconds” validity text uses bold, block styling
- ✅ Disclaimer block is centered with
text-xsand gray styling⚠️ eID instruction<p>is onlytext-gray-600—addtext-smandtext-centerfor higher prominence
🧹 Nitpick comments (3)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (3)
122-125: Reduce eID instruction font on small screens to avoid line breaks.Make the copy responsive so it shrinks on narrow viewports, per the issue requirements.
- <p className="text-gray-600"> + <p className="text-sm md:text-base text-gray-600"> Scan the QR code using your <a href={getAppStoreLink()}><b><u>eID App</u></b></a> to login </p>
157-158: Fix split closing tag.Minor JSX formatting nit to keep tags well-formed on one line.
- <span className="block font-light text-gray-600">Please refresh the page if it expires</span - > + <span className="block font-light text-gray-600">Please refresh the page if it expires</span>
167-167: US English spelling (optional): “centralized” vs “centralised”.If the product copy targets US English, prefer “centralized”.
- is stored in your own sovereign eVault, not on centralised + is stored in your own sovereign eVault, not on centralized
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx(1 hunks)
🔇 Additional comments (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (1)
162-169: Disclaimer styling aligns with Issue #347 (centered, smaller, lower-contrast).The update to text-center, text-xs, and text-gray-500 de-emphasizes the disclaimer appropriately while keeping it readable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
platforms/pictique/src/routes/(auth)/auth/+page.svelte (3)
84-84: Copy nit: use “log in” (verb) instead of “login” (noun).Also optional: prefer semantic
<strong>/CSS underline over<b>/<u>for consistency with Tailwind utilities.- Scan the QR code using your <b><u class="text-sm">eID App</u></b> to login + Scan the QR code using your <b><u class="text-sm">eID App</u></b> to log in
137-142: Lower-contrast disclaimer may miss WCAG AA at small sizes; consider slight bump.Keeps de-emphasis but improves readability on variable backgrounds.
- <p class="w-full rounded-md bg-white/60 p-4 text-center text-xs leading-4 text-black/40"> + <p class="w-full rounded-md bg-white/80 p-4 text-center text-xs leading-4 text-black/60">
144-146: Addrel="noopener noreferrer"to external link opened in a new tab.Prevents
window.openeraccess.- <a href="https://metastate.foundation" target="_blank"> + <a href="https://metastate.foundation" target="_blank" rel="noopener noreferrer">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/pictique/src/routes/(auth)/auth/+page.svelte(2 hunks)
🔇 Additional comments (1)
platforms/pictique/src/routes/(auth)/auth/+page.svelte (1)
90-95: Verify mobile tap target after font-size reduction.
text-baselowers label size; withpy-4the height should still be ~48px. Please confirm the button meets the 44×44px minimum on iOS/Android.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/blabsy/src/components/login/login-main.tsx (1)
20-30: Close SSE and add cleanup to prevent leaks/duplicate sign-ins
EventSourcestays open indefinitely; close it after successful sign-in and on error.Apply within this block:
- eventSource.onmessage = async (e): Promise<void> => { - const data = JSON.parse(e.data as string) as { token: string }; - const { token } = data; - console.log(token); - await signInWithCustomToken(token); - }; + eventSource.onmessage = async (e): Promise<void> => { + try { + const data = JSON.parse(e.data as string) as { token: string }; + const { token } = data; + await signInWithCustomToken(token); + } finally { + eventSource.close(); + } + }; + eventSource.onerror = (): void => { + eventSource.close(); + };Outside this range, keep it robust across unmounts:
// add to imports import { useEffect, useState, useRef } from 'react'; // inside component const esRef = useRef<EventSource | null>(null); // in watchEventStream, after creating the EventSource esRef.current = eventSource; // in useEffect, add cleanup useEffect(() => { getOfferData().catch(console.error); return () => esRef.current?.close(); }, []);
🧹 Nitpick comments (7)
platforms/blabsy/src/components/login/login-main.tsx (7)
14-17: Prefer relative SSE URL to avoid brittle env depsAvoid hard reliance on
NEXT_PUBLIC_BASE_URLon the client; relative paths work across dev/prod.- const sseUrl = new URL( - `/api/auth/sessions/${id}`, - process.env.NEXT_PUBLIC_BASE_URL - ).toString(); + const sseUrl = `/api/auth/sessions/${id}`;
31-37: Same here: fetch offer via relative pathSimplifies config and prevents runtime errors if the env var is unset.
- const { data } = await axios.get<{ uri: string }>( - new URL( - '/api/auth/offer', - process.env.NEXT_PUBLIC_BASE_URL - ).toString() - ); + const { data } = await axios.get<{ uri: string }>('/api/auth/offer');
93-99: Disable the deep-link CTA untilqris readyPrevents navigating to
#and improves a11y.- <a - href={qr ? getDeepLinkUrl(qr) : '#'} - className='px-6 py-3 bg-blue-600 text-white rounded-lg hover:bg-blue-700 transition-colors text-center' - > + <a + href={qr ? getDeepLinkUrl(qr) : undefined} + aria-disabled={!qr} + tabIndex={qr ? 0 : -1} + onClick={(e) => { if (!qr) e.preventDefault(); }} + className={`px-6 py-3 bg-blue-600 text-white rounded-lg transition-colors text-center ${qr ? 'hover:bg-blue-700' : 'opacity-50 pointer-events-none'}`} + >
100-105: Fix split closing tagMinor JSX formatting glitch; keep the closing
</span>on one line.- <span className="block font-light text-gray-600">Please refresh the page if it expires</span - > + <span className="block font-light text-gray-600">Please refresh the page if it expires</span>Also applies to: 116-121
79-83: Arbitrarycontent:string quoting nitTo avoid parser/minifier quirks with smart quotes, prefer single-quoted Tailwind arbitrary values.
- <h1 className='text-center text-3xl before:content-["See_what’s_happening_in_the_world_right_now."] lg:text-6xl lg:before:content-["Happening_now"]'> + <h1 className='text-center text-3xl before:content-[\'See_what’s_happening_in_the_world_right_now.\'] lg:text-6xl lg:before:content-[\'Happening_now\']'>Optional: render the visible copy as real text instead of
::beforefor better i18n/theming.
125-132: Delete commented-out W3DS logo blockSince the second logo is intentionally removed, drop dead code instead of commenting it out.
- {/* <div className='absolute right-0 rotate-90 top-1/2'> - <Image - src='/assets/w3dslogo.svg' - alt='W3DS logo' - width={100} - height={20} - /> - </div> */}
135-141: Disclaimer still looks emphasized; tone it down per Issue #347Remove bold and reduce size/contrast to truly de-emphasize.
- <div className='grid gap-3 text-center font-bold text-white/70'> + <div className='grid gap-3 text-center font-normal text-white/60 text-xs md:text-sm'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/blabsy/src/components/login/login-main.tsx(2 hunks)
🔇 Additional comments (4)
platforms/blabsy/src/components/login/login-main.tsx (4)
101-104: Confirm the “60 seconds” validity matches backend TTLMake sure the offer/session actually expires in 60s across platforms to avoid misleading copy.
Also applies to: 117-120
77-77: Alignment update LGTMCentering the right pane on large screens matches the issue’s alignment requirement.
84-86: Centered subheader LGTMThis aligns with the “center-align” requirement.
44-53: Guard browser-only APIs for SSR safetynavigator/window can be undefined during SSR — guard and return a safe default in non-browser environments. If this component is used in the Next.js App Router, add "use client" at the top of the file.
- const getAppStoreLink = () => { - const userAgent = navigator.userAgent || navigator.vendor || window.opera; + const getAppStoreLink = (): string => { + if (typeof window === 'undefined' || typeof navigator === 'undefined') { + return "https://play.google.com/store/apps/details?id=foundation.metastate.eid_wallet"; + } + const userAgent = navigator.userAgent || navigator.vendor || (window as any).opera;Verification note: root package.json showed no "next" (jq returned null) and a search found no "use client" in this file — confirm whether this package uses Next.js/App Router before adding the directive.
Description of change
Fixed metastate platforms requested changes
Issue Number
closes #347
Type of change
How the change has been tested
CI and Manual
Change checklist
Summary by CodeRabbit