Skip to content

Bugfix/fix auth bootstrap bounce#21

Closed
saadmoumou wants to merge 22 commits intomasterfrom
bugfix/fix-auth-bootstrap-bounce
Closed

Bugfix/fix auth bootstrap bounce#21
saadmoumou wants to merge 22 commits intomasterfrom
bugfix/fix-auth-bootstrap-bounce

Conversation

@saadmoumou
Copy link
Copy Markdown
Contributor

Summary

  • What does this PR change?

Why

  • Why is this change needed?

Checklist

  • Added/updated tests (if behavior changed)
  • npm run lint passes
  • npm run typecheck passes
  • npm test passes
  • npm run build passes
  • Added a changeset (npx changeset) if this affects consumers

Notes

  • Anything reviewers should pay attention to?

Zaiidmo and others added 22 commits March 3, 2026 12:37
- Replace non-standard ci.yml with standardized release-check.yml and pr-validation.yml
- Create dependabot.yml for automated dependency management (weekly, 5 PR limit)
- Add sonarqube_mcp.instructions.md for SonarQube MCP server guidance
- Ensure consistent GitHub Actions versions (v4 for checkout and setup-node)
- Configure standardized Node versions (v22 for release, v20 for validation/publish)
- Pin SonarQube actions to commit SHA for security hardening
- Standardize branch triggers ([master, main] for release, [develop] for validation)

This aligns AuthKit-UI with the standardized CI/CD pattern used across all @ciscode/* packages.
- Add comprehensive instruction files in .github/instructions/
- Includes copilot, testing, bugfix, features, general guidelines
- Standardize documentation across all repositories
- Remove deprecated instruction files from .github/ root
- Consolidate all docs in .github/instructions/ directory
- Improve documentation organization
…ckages

- Replace git tag --list strategy with package.json-driven tag validation
  in all 16 publish workflows; use git rev-parse to verify the exact tag
  exists rather than guessing the latest repo-wide tag
- Update error guidance to reflect feat/** → develop → master flow
- Standardize dependabot to npm-only, grouped, monthly cadence across
  all 16 packages; remove github-actions ecosystem updates
- Add missing dependabot.yml to AuthKit-UI, ChartKit-UI, HealthKit,
  HooksKit, paymentkit, StorageKit
…ript

- Fix typecheck: add paths aliases to resolve @types/react dual-version conflict
- Fix test:cov script: use 'vitest run --coverage' (was watch mode)
- Add vitest coverage config: include src/**, exclude models/assets
- Add tests/utils/errorHelpers.test.ts: 9 tests covering extractHttpErrorMessage
- Add tests/components/SocialButton.test.tsx: 3 tests for SocialButton render
- Add tests/components/ProfilePage.test.tsx: 7 tests (load, edit, save, cancel, error toast)
- Add tests/pages/auth/authPages.test.tsx: tests for ForgotPassword, ResetPassword, VerifyEmail, GoogleCallback, SignIn, SignUp pages
- Add tests/exports.test.ts: smoke test for src/main/app re-exports

Coverage: 28.81%% -> 85%% statements
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot

See analysis details on SonarQube Cloud

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR mixes an auth bootstrap/navigation bugfix with broad library/tooling updates (tests, coverage, lint/format, and CI/CD workflows) for @ciscode/ui-authentication-kit.

Changes:

  • Updates auth provider/pages behavior and RBAC permission hooks; adds new AuthConfig options (custom signup URL/fields/endpoint/payload transform).
  • Adds a substantial new Vitest test suite plus coverage configuration.
  • Overhauls CI workflows (PR validation, release checks with Sonar, publishing flow), and adds repo engineering docs/instructions.

Reviewed changes

Copilot reviewed 37 out of 40 changed files in this pull request and generated 15 comments.

Show a summary per file
File Description
vitest.config.ts Enables V8 coverage and configures include/exclude paths.
tsconfig.json Adds TypeScript paths overrides for React-related modules.
tests/utils/errorHelpers.test.ts Adds unit tests for HTTP/axios error message extraction.
tests/pages/auth/authPages.test.tsx Adds page-level tests for auth flows (signin/signup/reset/verify/callback).
tests/exports.test.ts Adds a “smoke test” intended to validate package exports.
tests/context/RbacContext.test.tsx Updates RBAC tests to use useCanAny.
tests/components/SocialButton.test.tsx Adds component tests for SocialButton.
tests/components/RequirePermissions.test.tsx Updates RequirePermissions tests to include useCanAny behavior.
tests/components/ProfilePage.test.tsx Adds ProfilePage behavior tests (load/edit/save flows).
src/utils/errorHelpers.ts Tightens typing in isAxiosError helper.
src/utils/colorHelpers.ts Refactors Tailwind prefix typing approach.
src/utils/attachAuthInterceptor.ts Improves typing for _retry flag on axios request config.
src/providers/AuthProvider.tsx Adjusts auth bootstrap, routes, interceptor wiring, and types.
src/pages/auth/VerifyEmailPage.tsx Removes unused Tailwind class output from toTailwindColorClasses.
src/pages/auth/SignUpPage.tsx Adds customizable signup fields/endpoint/payload transform and redirect behavior.
src/pages/auth/SignInPage.tsx Reads provider-level errors from sessionStorage via state initializer; supports custom signup URL.
src/pages/auth/ResetPasswordPage.tsx Replaces any catch with unknown.
src/models/AuthConfig.ts Adds CustomField and new AuthConfigProps options for signup customization.
src/hooks/useAbility.ts Adds useCanAny hook.
src/context/RbacContext.ts Fixes hook usage by calling permission/role hooks unconditionally.
src/components/RequirePermissions.tsx Fixes hook rule violations by unconditionally calling permission/role hooks.
src/components/InlineError.tsx Changes dismissal logic to avoid setState-in-effect patterns.
package.json Adds lint/format/typecheck scripts, bumps versions, and updates tooling dependencies.
eslint.config.mjs Updates ignore patterns and react-hooks-related rules; adjusts TypeScript/React lint rules.
.prettierrc.json Expands formatting settings (tabs, spacing, EOL).
.github/workflows/release-check.yml Adds master PR “release check” pipeline (quality/test/build/sonar).
.github/workflows/publish.yml Reworks publish workflow to run on master pushes and validate tags.
.github/workflows/pr-validation.yml Adds PR validation workflow for develop PRs.
.github/workflows/ci.yml Removes the previous combined CI workflow.
.github/instructions/testing.instructions.md Adds testing standards and patterns documentation.
.github/instructions/sonarqube_mcp.instructions.md Adds SonarQube MCP usage guidelines documentation.
.github/instructions/general.instructions.md Adds general engineering guidelines for the module.
.github/instructions/features.instructions.md Adds feature development workflow documentation.
.github/instructions/copilot-instructions.md Adds module-specific Copilot guidance documentation.
.github/instructions/components.instructions.md Adds component development standards documentation.
.github/instructions/bugfix.instructions.md Adds bugfix process documentation.
.github/dependabot.yml Enables Dependabot for npm dependencies.
.github/CODEOWNERS Adds default code ownership.
.changeset/authkit_ui_71368.md Adds a changeset entry for a minor release.

Comment thread tests/exports.test.ts
Comment on lines +1 to +4
/**
* Smoke test: importing from the package entry point covers
* src/index.ts and src/main/app.ts re-export lines.
*/
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file claims to be a smoke test for the package entry point, but the imports are from internal source paths (e.g., ../src/main/app, ../src/components/ProfilePage). That won’t catch missing/incorrect public exports. Import from the published entry module (e.g., ../src/index) and assert the symbols from there.

Copilot uses AI. Check for mistakes.
Comment on lines +89 to +93
if (signUpTransformPayload) {
payload = signUpTransformPayload(payload);
}

// 1) Register the user dynamically
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior here (optional signUpTransformPayload and posting to configurable signUpEndpoint) is consumer-facing, but the added SignUpPage tests don’t cover these branches (including the custom redirect when signUpEndpoint !== '/api/auth/register'). Please add tests exercising the transform + endpoint/redirect logic to prevent regressions.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +12 to +14
import { ForgotPasswordPage } from "../pages/auth/ForgotPasswordPage";
import { GoogleCallbackPage } from "../pages/auth/GoogleCallbackPage";
import { ResetPasswordPage } from "../pages/auth/ResetPasswordPage";
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GoogleCallbackPage is also used for the Microsoft OAuth callback route in this provider, but the page renders Google-specific UI copy (“Finishing Google sign-in…”). Consider making the callback page provider-agnostic (name + copy), or adding a separate Microsoft callback page, so the UX and semantics match the provider.

Copilot uses AI. Check for mistakes.
Comment on lines 3 to 7
on:
push:
tags:
- "v*.*.*"
branches:
- master
workflow_dispatch:
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release docs indicate publishing is triggered by pushing a git tag on master, but this workflow triggers on any push to master. Consider switching the trigger back to push.tags: ["v*.*.*"] (or aligning docs + release process) to avoid accidental publishes on ordinary merges.

Copilot uses AI. Check for mistakes.
Comment on lines 46 to 49
/* ── Google OAuth callback component (inside AuthProvider so it can touch state) ── */
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const GoogleOAuthCallback: React.FC = () => {
const location = useLocation();
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GoogleOAuthCallback is defined but not used (eslint is disabled to silence it). Either wire this component into the callback route or remove it to avoid dead code and confusion about the actual OAuth callback handling.

Copilot uses AI. Check for mistakes.
Comment thread package.json
{
"name": "@ciscode/ui-authentication-kit",
"version": "1.0.13",
"version": "1.0.15",
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package uses Changesets for versioning (see docs/RELEASE.md). Manually bumping package.json version here can conflict with changeset version output and the tag-based publishing flow; prefer leaving version unchanged in PRs and letting Changesets apply it during release prep.

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +77
run: npm run lint --if-present 2>/dev/null || true

- name: Test
run: npm test --if-present 2>/dev/null || true
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm run lint and npm test failures are being ignored (|| true), which can allow publishing broken builds. Remove the error suppression so the workflow fails fast if lint/tests fail (or gate publish behind the same verify script used locally).

Suggested change
run: npm run lint --if-present 2>/dev/null || true
- name: Test
run: npm test --if-present 2>/dev/null || true
run: npm run lint --if-present
- name: Test
run: npm test --if-present

Copilot uses AI. Check for mistakes.
Comment on lines +151 to +154
-Dsonar.tests=test
-Dsonar.test.inclusions=**/*.spec.ts,**/*.test.ts
-Dsonar.exclusions=**/node_modules/**,**/dist/**,**/coverage/**,**/*.d.ts
-Dsonar.coverage.exclusions=**/*.spec.ts,**/*.test.ts,**/index.ts
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sonar is configured with -Dsonar.tests=test, but this repo’s test folder is tests/. Also sonar.test.inclusions excludes *.test.tsx, so React component tests won’t be detected. Update these paths/patterns so coverage & test metrics are accurate.

Suggested change
-Dsonar.tests=test
-Dsonar.test.inclusions=**/*.spec.ts,**/*.test.ts
-Dsonar.exclusions=**/node_modules/**,**/dist/**,**/coverage/**,**/*.d.ts
-Dsonar.coverage.exclusions=**/*.spec.ts,**/*.test.ts,**/index.ts
-Dsonar.tests=tests
-Dsonar.test.inclusions=**/*.spec.ts,**/*.spec.tsx,**/*.test.ts,**/*.test.tsx
-Dsonar.exclusions=**/node_modules/**,**/dist/**,**/coverage/**,**/*.d.ts
-Dsonar.coverage.exclusions=**/*.spec.ts,**/*.spec.tsx,**/*.test.ts,**/*.test.tsx,**/index.ts

Copilot uses AI. Check for mistakes.
Comment on lines 117 to 123
useEffect(() => {
const init = async () => {
if (accessToken) {
setUser(decodeToken(accessToken));
setBooting(false);
return;
}

Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bootstrap effect reads accessToken and sets user, but accessToken is intentionally omitted from the dependency array. This breaks updates when the interceptor refreshes the token (it calls setAccessToken but not setUser), leaving user stale/null. Add a separate effect that derives user from accessToken, or include accessToken in deps with appropriate guards.

Copilot uses AI. Check for mistakes.
Comment on lines +183 to +186
Object.defineProperty(window, 'location', {
value: { ...window.location, replace: vi.fn(), search: '?accessToken=tok&refreshToken=ref' },
writable: true,
});
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding window.location via Object.defineProperty(window, 'location', ...) often throws in JSDOM because location is not configurable. Prefer spying/mocking window.location.replace (e.g., vi.spyOn(window.location, 'replace')) or using vi.stubGlobal/jsdom.reconfigure patterns to avoid flaky tests.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants