Skip to content

Feat/reusing sdk components#68

Open
michael-on-code wants to merge 9 commits into
mainfrom
feat/reusing-sdk-components
Open

Feat/reusing sdk components#68
michael-on-code wants to merge 9 commits into
mainfrom
feat/reusing-sdk-components

Conversation

@michael-on-code

Copy link
Copy Markdown
Contributor

Checklist

  • Tests were added/updated according to the feature/bugfix/change made
  • Version was rolled according to semver requirements
  • API endpoints openapi schema was updated if applicable

Changes

  • course tab components & hooks all now imported from the sdk
  • profile > pages components & hooks all now imported from the sdk

@mamigot

mamigot commented Apr 16, 2026

Copy link
Copy Markdown
Member

Claude finished @michael-on-code's task —— View job


E2E Coverage Review ✅

Reviewing PR #68 for e2e test coverage requirements.

Task List

  • Dismiss stale REQUEST_CHANGES reviews from myself
  • Identify what changed (git diff)
  • Run mechanical coverage check
  • Read current coverage state
  • Apply decision logic and determine review outcome
  • Post GitHub review (approve or request changes)

Analysis

Changed files: No new app/**/page.tsx routes were added. All changes are modifications to existing routes and deletion of local components that moved to the SDK.

Key findings:

  • All modified page.tsx files already have e2e coverage in journey 5 (course-content-tabs) and various profile journeys
  • Many local components/hooks deleted as they moved to SDK (course-outline, edx-iframe, etc.)
  • No new routes that need coverage updates
  • This matches CASE E from the decision logic

Coverage verification:

  • coverage.json shows 215 checkpoints with 100% coverage
  • All modified routes are already mapped to existing journeys:
    • Course content tabs → Journey 5 (course-content-tabs)
    • Profile pages → Journeys 6-12 (profile-*)
  • No coverage regression detected

✅ Review Result: APPROVED

No new app routes were introduced. All existing coverage is intact and no updates to coverage files are required.


@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

Code Quality Review

  • Quality: Clean SDK migration — net -1,823 lines by replacing local implementations with shared @iblai/iblai-js components. The useCallback addition in use-personnalized-catalog.ts is a good stability improvement.
  • Issues:
    • Multiple // @ts-ignore on SDK imports (CourseContentLayout, useGetDepartmentMemberCheckQuery, etc.) means TypeScript won't catch API drift if the SDK changes its exports. Prefer // @ts-expect-error or push for proper type exports from the SDK package.
    • .claude/settings.json is committed to the repo — verify this is intentional and doesn't expose environment-specific config.
    • Tests now mock the SDK container wholesale (capturedProps pattern), which only verifies props are forwarded but not that the SDK component renders correctly in context. Acceptable for unit tests, but worth noting if SDK integration behaviour changes.
  • Suggestions: Add a d.ts shim or configure compilerOptions.paths for @iblai/iblai-js so // @ts-ignore blocks can be removed incrementally.

Summary

Solid consolidation PR that reduces maintenance burden by delegating UI responsibility to the shared SDK. The main risk is the untyped SDK surface — if SDK exports change, errors will surface at runtime rather than at build time.

@claude

claude Bot commented Apr 16, 2026

Copy link
Copy Markdown

UI/UX Review

  • Accessibility: SDK components absorb all ARIA responsibility — verify SharedCourseContentLayout and profile components preserve labels like "Open course outline" and maintain keyboard-navigable tab order. No new a11y regressions visible in the wrapper code.
  • UX Concerns: The tabHrefTemplate forum→discussion remap (tab === 'forum' ? 'discussion' : tab) is a silent alias — confirm deep-links and back-navigation resolve correctly so users are not dropped to a 404.
  • Suggestions: Smoke-test on a narrow viewport to confirm the SDK's mobile sidebar behaviour matches the previous xl:hidden drawer. Verify CourseContentLoading skeleton matches the visual weight of the removed custom skeleton.

Summary

Clean delegation to SDK components with no obvious UX regressions in the wrapper layer. The main risk is unverified SDK accessibility/responsiveness behaviour that was previously explicit in local code — a short manual pass on mobile and screen reader is the key outstanding check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants