feat: add Finny visibility and access gating (closes #239) - #254
Conversation
- Show Finny only when transactions.length > 5 (DataContext) - Hide Finny entirely for 5 or fewer transactions - Show sign-in prompt only for unauthenticated users, no chatbot logic runs - Scoped to visibility/access control only
👷 Deploy request for finnboard0 pending review.Visit the deploys page to approve it
|
✅ Deploy Preview for finnboard ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thanks for the quick turnaround on this @hemag111 , looks good overall and the core gating logic matches the issue requirements. Transaction check looks solid, One thing I want to double check before approving: right now the Finny trigger button shows up for any user with more than 5 transactions, even if they're logged out. Clicking it opens the panel and shows the sign in message, which technically satisfies "if opened, show the sign in message," but I want to confirm that's what we want vs hiding the entry point completely until the user is both authenticated and has enough transactions. Can you clarify which behavior was intended? A couple smaller things, not blockers: The modal doesn't close on Escape and there's no focus trap, might be worth a follow up a11y pass later. No tests were added for the gating logic. Since this is exactly the kind of thing that can silently regress, a quick test for the 5 vs 6 transaction boundary and the logged out branch would give us more confidence. Also just a nit, MIN_TRANSACTIONS_TO_SHOW is only defined in Finny.jsx right now. Fine for a single use case, but if FinnyPanel or anything else ever needs that same threshold we should move it to a shared constants file so they don't drift apart. Nice work overall, just want clarity on the auth vs visibility question before merging. |
|
Good question — I intentionally kept the button visible whenever transactions.length > 5, regardless of auth state, and gated the content on open rather than the entry point itself. That was based on this line in the issue: "If a user is not signed in and Finny is opened/triggered... display a simple message" — which implies the trigger is reachable while logged out, otherwise that flow couldn't happen. Happy to switch to fully hiding it for logged-out users if you'd prefer that instead — just let me know which you want and I'll update it. For the other points:
|
|
@hemag111 Thanks for explaining the reasoning, that makes sense and matches the issue text, so let's keep the trigger visible for anyone with >5 transactions regardless of auth state. Everything else sounds good, go ahead with the Escape-to-close, focus trap, tests, and shared constants file as planned. |
- Extract gating logic into src/lib/finnyGating.js (shouldShowFinny, isFinnyAuthorized) for testability - Move MIN_TRANSACTIONS_TO_SHOW into shared src/lib/finnyConstants.js - Add Escape-to-close and basic focus handling to FinnyPanel - Add tests for transaction boundary (5 vs 6) and auth gating
|
@khanirfan18 Thanks ,I’ve pushed the follow-up change. The logged-out Finny panel now has actionable Sign in and Sign up links, while keeping the trigger visible for users with more than 5 transactions as discussed. The previous tests and production build passed successfully. |
khanirfan18
added
enhancement
|
hey @hemag111, really glad you took the time to contribute! your first PR just got merged and that genuinely means a lot to us 🎉 hope this is just the beginning. feel free to pick up more issues whenever you're up for it, and if you're enjoying the project a ⭐ on the repo would be awesome |
2 participants
Closes #239
Changes
transactions.length > 5(read fromDataContext)Layout.jsxScope
Visibility + access control only, per issue — chatbot Q&A/AI integration left as placeholder for a future issue.
Testing
npm run lintandnpm run buildpass. Manually verified: hidden with ≤5 transactions, visible with >5, sign-in message shown when logged out, placeholder shown when logged in.