[WEB-3071] smart-on-fhir launch endpoint

[krystophv]

WEB-3071 initial work for smart-on-fhir launch endpoint

[krystophv]

/deploy dev

[tidebot]

krystophv updated values.yaml file in dev1

[tidebot]

krystophv updated flux policies file in dev1

[tidebot]

krystophv deployed blip WEB-3071-smart-on-fhir-launch branch to dev1 namespace

[krystophv]

/deploy dev

[tidebot]

krystophv updated values.yaml file in dev1

[tidebot]

krystophv updated flux policies file in dev1

[tidebot]

krystophv deployed blip WEB-3071-smart-on-fhir-launch branch to dev1 namespace

[krystophv]

/deploy dev

[tidebot]

krystophv updated values.yaml file in dev1

[tidebot]

krystophv updated flux policies file in dev1

[tidebot]

krystophv deployed blip WEB-3071-smart-on-fhir-launch branch to dev1 namespace

[henry-tp-public]

Code Review Summary

This is a comprehensive and well-structured implementation of Smart-on-FHIR integration. The code follows established patterns and demonstrates good understanding of the existing codebase architecture.

Key Strengths:

• ✅ Proper use of Redux selectors and state management patterns
• ✅ Good performance optimizations with useMemo and createSelector
• ✅ Comprehensive error handling with user-friendly messages
• ✅ Consistent prop threading throughout the component hierarchy
• ✅ Well-documented API methods

Main Areas for Improvement:

1. Security: Add validation/encryption for sensitive parameters in sessionStorage
2. Code Consistency: Use existing selectors instead of direct state access
3. Component Responsibility: Reduce direct Redux dependencies in presentational components
4. PropTypes Consistency: Standardize required/optional prop declarations

Security Priority:

The session storage of sensitive FHIR parameters should be addressed before merging, as this could pose security risks in production environments.

Overall, this is solid work that integrates well with the existing codebase. The suggested improvements are primarily around security hardening and maintaining consistency with established patterns.

[henry-tp-public]

Security Concern: Session Storage Risk

Using sessionStorage for sensitive FHIR parameters without encryption poses a security risk. Consider encrypting these sensitive parameters or using more secure storage mechanisms. At minimum, add validation to ensure stored values haven't been tampered with.

// Consider adding validation/encryption
const iss = validateAndDecrypt(sessionStorage.getItem('smart_iss')) || urlParams.get('iss');

[henry-tp-public]

Security: URL Parameter Validation Needed

Direct URL parameter extraction and storage without validation could be vulnerable to XSS or injection attacks. Add input validation for URL parameters (iss, launch, correlation_id).

// Add validation helpers
const validateFhirIss = (iss) => {
  // Validate iss format and whitelist known domains
  return /^https:\/\/[\w.-]+\/fhir/.test(iss) ? iss : null;
};

const iss = validateFhirIss(urlParams.get('iss'));
const launch = urlParams.get('launch')?.replace(/[^a-zA-Z0-9-_]/, '') || null;

[henry-tp-public]

Code Consistency: Use Existing Selector

Using direct state property access with magic strings is inconsistent with the rest of the codebase. Use the existing selectIsSmartOnFhirMode(state) selector for consistency.

// Replace this line:
// isSmartOnFhirMode: state.blip.smartCorrelationId !== null,
// With:
isSmartOnFhirMode: selectIsSmartOnFhirMode(state),

[henry-tp-public]

PropTypes Consistency Issue

Inconsistent PropTypes declarations across chart components. Some mark isSmartOnFhirMode as isRequired while others don't for the same prop. Consider standardizing this - if the prop is always passed down, it should consistently be marked as isRequired.

Also appears in: app/components/chart/daily.js, app/components/chart/trends.js, etc.

[henry-tp-public]

Code Complexity: Extract Method

The render method has grown quite complex with multiple nested conditions for Smart-on-FHIR handling. Consider extracting the Smart-on-FHIR URL parameter logic into a separate method like handleSmartOnFhirParams() for better readability and maintainability.

[henry-tp-public]

Good Practice: Selector Performance

✅ Excellent use of createSelector for the new selectIsSmartOnFhirMode selector. This provides proper memoization and follows the established patterns in the codebase.

[henry-tp-public]

Error Handling Enhancement

The error handling covers multiple states but could benefit from retry logic for network failures and clearer user guidance for recoverable errors. Consider adding:

1. Retry mechanism for API failures
2. More specific error messages for different failure types
3. Recovery actions where possible

[henry-tp-public]

Component Responsibility Concern

The Footer component now depends on Redux state management, which breaks component purity. Consider passing isSmartOnFhirMode as a prop from the parent component instead of using useSelector directly to maintain better separation of concerns.

[henry-tp-public]

Performance Optimization

✅ Good use of useMemo for expensive operations. The component re-rendering logic is well-optimized and follows React best practices.

[henry-tp-public]

Code Organization: Extract Utility

The Zendesk hiding logic with retry mechanism could be extracted into a utility function for reusability across the application, especially if other components need to interact with Zendesk in Smart-on-FHIR mode.

// utils/zendesk.js
export const hideZendeskWidget = (maxAttempts = 50) => {
  let attempts = 0;
  let timeoutId;
  
  const checkZendesk = () => {
    if (window.zE) {
      window.zE('webWidget', 'hide');
      return;
    }
    
    attempts++;
    if (attempts < maxAttempts) {
      timeoutId = setTimeout(checkZendesk, 100);
    }
  };
  
  checkZendesk();
  return () => timeoutId && clearTimeout(timeoutId);
};

[Copilot]

Pull Request Overview

This PR implements initial Smart-on-FHIR integration to enable launching Tidepool from Electronic Health Record (EHR) systems. The implementation focuses on handling the SMART authentication flow and displaying patient data in a specialized mode.

• Smart-on-FHIR authentication flow with correlation ID tracking
• Patient lookup functionality using MRN and date of birth from SMART tokens
• Specialized UI mode that hides certain features when in Smart-on-FHIR context

Reviewed Changes

Copilot reviewed 77 out of 79 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
app/pages/smartonfhir/	New Smart-on-FHIR page and error components for patient matching scenarios
app/redux/reducers/	New reducers for Smart-on-FHIR data and correlation ID state management
app/redux/actions/	New actions for Smart-on-FHIR authentication and patient fetching
app/keycloak.js	Updated to handle Smart-on-FHIR client configuration and token parsing
app/routes.js	Added new route and authentication guard for Smart-on-FHIR flow
test/	Comprehensive test coverage for new Smart-on-FHIR functionality

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

[nitpick] The function returns undefined when not in Smart-on-FHIR mode, but the calling code uses spread operators that could handle empty objects better. Consider returning an empty object {} instead of undefined to simplify the calling code and reduce conditional logic.

Suggested change

	return isSmartOnFhir ? { isSmartOnFhir: true } : undefined;
	return isSmartOnFhir ? { isSmartOnFhir: true } : {};

[Copilot]

[nitpick] The Zendesk hiding logic uses a polling approach with hardcoded values. Consider using a more robust approach like a MutationObserver to detect when Zendesk is loaded, or make the polling parameters configurable constants.

Suggested change

	let attempts = 0;
	const maxAttempts = 50; // 5 seconds max (50 * 100ms)
	let timeoutId;
	const checkZendesk = () => {
	if (windowObj.zE) {
	windowObj.zE('webWidget', 'hide');
	return;
	}
	attempts++;
	if (attempts < maxAttempts) {
	timeoutId = setTimeout(checkZendesk, 100);
	}
	};
	checkZendesk();
	return () => {
	if (timeoutId) clearTimeout(timeoutId);
	let observer;
	let zendeskHidden = false;
	const hideZendesk = () => {
	if (windowObj.zE && !zendeskHidden) {
	windowObj.zE('webWidget', 'hide');
	zendeskHidden = true;
	}
	};
	// Try to hide immediately in case zE is already available
	hideZendesk();
	// Observe DOM mutations to detect when Zendesk widget is loaded
	observer = new MutationObserver(() => {
	hideZendesk();
	});
	observer.observe(document.body, { childList: true, subtree: true });
	return () => {
	if (observer) observer.disconnect();

[Copilot]

[nitpick] Both error conditions use the same error message ERR_SMARTONFHIR_CLINICIAN_NO_CLINICS but have different underlying causes (fetch failure vs. no clinics available). Consider using distinct error messages to help with debugging and user experience.

[Copilot]

The code uses crypto.randomUUID() without checking browser support. This API is not available in all browsers and could cause runtime errors. Consider adding a fallback or polyfill, or check for API availability before use.

[Copilot]

[nitpick] The function accesses both sessionStorage and window.location directly, making it difficult to test and potentially causing issues in non-browser environments. Consider passing these as parameters or using dependency injection for better testability.