feat: add runtime PAT entry and user avatar#15
Conversation
twoGiants
left a comment
There was a problem hiding this comment.
First batch. Second batch follows after the next call 🤙 😸
There was a problem hiding this comment.
Lets update this after approval.
Simplify EmptyState to a single isCreateDisabled prop. When disabled, show only the PAT hint text instead of both messages, and remove the redundant Tooltip from the disabled button. Remove Tooltip from the table-view Create button in FunctionsListPage as well. Protect FunctionCreatePage against unauthenticated access via URL by showing a warning alert and hiding the form when no PAT is stored. Clean up UserAvatar: remove all useCallback wrappers, remove unused logout, use guard pattern in readStoredUser, extract usePatModal hook, shrink modal to small variant, disable Cancel and modal close while validating. Extract shared errorMessage utility. Remove stale question from features.json and update i18n keys. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Matej Vašek <matejvasek@gmail.com>
twoGiants
left a comment
There was a problem hiding this comment.
Ok, you and Claude did great! I would approve and merge.
Can you open a cleanup follow up with the fixes I mention in the comments below?
I'd like to have this PR in my branch for a full e2e flow test.
| it('disables Cancel button while validating', async () => { | ||
| const user = userEvent.setup(); | ||
| let resolveConnect: () => void; | ||
| mockFetchUserInfo.mockReturnValue( | ||
| new Promise<void>((resolve) => { | ||
| resolveConnect = resolve; | ||
| }), | ||
| ); | ||
|
|
||
| renderWithContext(<UserAvatar enableReconnect />); | ||
|
|
||
| await user.type(screen.getByLabelText('Personal Access Token'), 'ghp_slow'); | ||
| await user.click(screen.getByRole('button', { name: 'Connect' })); | ||
|
|
||
| expect(screen.getByRole('button', { name: 'Cancel' })).toBeDisabled(); | ||
|
|
||
| resolveConnect!(); | ||
| }); |
There was a problem hiding this comment.
No need to resolve as we don't assert after resolution. You can remove resolveConnect and change the mockReturnValue to new Promise(() => {}). Makes the test a bit simpler.
|
|
||
| export default function FunctionCreatePage() { | ||
| const { t } = useTranslation('plugin__console-functions-plugin'); | ||
| const isConnected = !!sessionStorage.getItem(PAT_KEY); |
There was a problem hiding this comment.
No, this is a hack. Use context for that. Same pattern as in the List page:
function useFunctionListPage(): {
// ...
isConnectedToForge: boolean;
} {
const isConnectedToForge = useContext(ForgeConnectionContext).isActive;| jest.mock('../components/UserAvatar', () => ({ | ||
| UserAvatar: ({ enableReconnect }: { enableReconnect: boolean }) => ( | ||
| <span data-testid="user-avatar">{enableReconnect ? 'reconnect' : 'no-reconnect'}</span> | ||
| ), | ||
| })); |
There was a problem hiding this comment.
I don't think you have to mock the component here, only the services. If you can do without mocking it, do it without.
| jest.mock('../components/UserAvatar', () => ({ | ||
| UserAvatar: ({ enableReconnect }: { enableReconnect: boolean }) => ( | ||
| <span data-testid="user-avatar">{enableReconnect ? 'reconnect' : 'no-reconnect'}</span> | ||
| ), | ||
| })); |
There was a problem hiding this comment.
Don't mock if you don't have to.
Replace compile-time PAT injection (webpack DefinePlugin, __GITHUB_PAT__) with runtime entry via a modal dialog. PAT is stored in sessionStorage and read lazily per-request through a cached Octokit getter. UserAvatar component renders in ListPageHeader on every page. On the list page it is clickable (opens PatModal to connect or reconnect); on create and edit pages it is read-only. PatModal auto-opens on first visit when no PAT is stored. Cancel and modal close are disabled while validation is in flight. ForgeConnectionProvider context lets FunctionsListPage react when the user connects. EmptyState shows a PAT hint and disables the Create button when not connected. FunctionCreatePage hides the form entirely and shows a warning alert when accessed without a PAT. GithubService refactored: single Octokit instance with a cached getter that re-creates it only when the token changes. fetchUserInfo added to SourceControlService interface (forge-agnostic). GitHubUser renamed to ForgeUser. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Matej Vašek <matejvasek@gmail.com>
d8e19cb to
1e51bf1
Compare
Summary
authStrategythat reads the token lazily per-request from sessionStoragePatModalcomponent (PAT entry with validation feedback) andUserAvatarcomponent (connection status in page header)useUserAvatarhook managing PAT state, sessionStorage persistence, and modal lifecycleEmptyStatewith hint text and disabled Create button when unauthenticatedTest plan
yarn test), zero lint errors (yarn lint), zero type errors insrc/(npx tsc --noEmit)__GITHUB_PAT__references remain insrc/orwebpack.config.tsDemo
Screencast.From.2026-04-27.22-09-24.mp4
🤖 Generated with Claude Code