Project: doctolib-doctolib · Branch: .invalid · Mode: · Model: claude-opus-4-7 Stats: 1 user messages, 18 tool calls, tools used: Bash, Read. Conversation Turn 1 — User Review the following code for simplification opportunities in PR FHIR-2184. Focus only on recently changed code. Changed files (worktree at /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184): 1. app/controllers/configuration/gdt_exports_controller.rb (lines 8-11): def new @organization_id = current_account_admin_organizations.find { |org| Ehr::Api::Organization.ehr?(organization_id: org.id) }&.id end 2. app/components/react/configuration/gdt_export_creation.rb (lines 4-6): def to_props(organization_id: nil) to_camelized_props({ organization_id: organization_id }) end 3. TypeScript GDTExportCreation.tsx — new type at line 19: type CreateDeviceButtonProps = { organizationId?: number } And at line 47: organization_id: organizationId ?? 0, 4. GDTExportCreation component signature at line 54: const GDTExportCreation = ({ organizationId }: { organizationId?: number }) => { Note: GDTExportCreation has an inline type { organizationId?: number } while CreateDeviceButton uses a named type CreateDeviceButtonProps. This is inconsistent — either both should be inline or both named. Context: - current_account_admin_organizations returns organizations with eager loading and ordering - The .find { |org| Ehr::Api::Organization.ehr?(...) } call means potentially N DB queries (one per org) - organizationId ?? 0 — 0 is used as a fallback for missing organization, which may silently cause API failures Check for: - Consistency in type declaration approach (inline vs named type) - Any simplification opportunities in the Ruby prop component - Whether ?? 0 is the right pattern vs returning early or showing an error Output using Conventional Comments format. Turn 1 — Assistant I'll review the code carefully. Let me first inspect the actual files in the worktree to understand the full context. Tools used: - Bash: ls /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/ 2>&1 | head -20 Tool results: → result (ok): AGENTS.md app babel.config.js backend.dependencies.codeowners.yml bin catalog-info.yaml ci CLAUDE.md ⇒ AGENTS.md CODEOWNERS CODEOWNERS.local component_backlog.yml config config.ru CONTRIBUTING.md db dev-env-setup-wsl dev-tools doc docker docker-compose.linux.yml Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && git diff master --stat 2>&1 | head -30 Tool results: → result (ok): .cherry/freemium.ts | 1 - .../migrate-patient-accounts-test-factories.md | 1 - .claude/commands/remove-eslint-disablement.md | 8 + .codeowners_by_team/boss | 1 + .codeowners_by_team/byp | 5 + .codeowners_by_team/cat | 1 + .codeowners_by_team/cusco | 3 - .codeowners_by_team/ditto | 2 + .codeowners_ …(truncated, 1341 more chars) The worktree appears to be on a branch with many unrelated changes. Let me focus on the specific files mentioned in the task. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && git log --oneline -5 2>&1 Tool results: → result (ok): 42019749ed42 fix(FHIR-2184): make organizationId optional with ?? 0 fallback 47a179a6321e fix(FHIR-2184): remove duplicate mount div causing organizationId to be undefined 2de1ef1b7bcd refactor(FHIR-2184): use organizationId prop directly, drop AccountContext fallback ae932378b39c fix(FHIR-2184): resolve EHR-eligible org server-side for medical device creation a491bd7833d4 fix(RATP-1884): flaky skipper (#241155) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && git diff a491bd7833d4..42019749ed42 -- 'app/controllers/configuration/gdt_exports_controller.rb' 'app/components/react/configurat …(truncated, 128 more chars) Tool results: → result (ok): diff --git a/app/components/react/configuration/gdt_export_creation.rb b/app/components/react/configuration/gdt_export_creation.rb index 249d980c7cf9..1e05ac28c84e 100644 --- a/app/components/react/configuration/gdt_export_creation.rb +++ b/app/components/react/configuration/gdt_export_creation.rb @@ -1,3 +1,7 @@ class React::Configuration::GdtExportCreation < React::Component self.selector = '.gdt-export-creation' + + def to_props(organization_id: nil) + to_camelized_props({ organizatio …(truncated, 523 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && find . -name 'GDTExportCreation.tsx' 2>/dev/null | head -5 Tool results: → result (ok): ./packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && git diff a491bd7833d4..42019749ed42 -- 'packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx' 2>&1 Tool results: → result (ok): diff --git a/packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx b/packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx index 36125a253788..554c19eccefa 100644 --- a/packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx +++ b/packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx @@ -10,20 +10,18 @@ import { useMedicalDevicesCreate } from '@doctolib/equipment_configuration_sdk/p import type …(truncated, 2709 more chars) Now I have the full picture. Let me also check how other React components in the project handle to_props (to gauge the convention) and how current_account_admin_organizations is set up for N+1 considerations. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -rn "current_account_admin_organizations" --include='.rb' -l 2>/dev/null | head -10 Tool results: → result (ok): app/policies/doctor_authorizations.rb app/policies/configuration/hospital_unit_policy.rb app/policies/configuration/practitioner_substitute_policy.rb app/policies/configuration/secure_access/organization_policy.rb app/policies/v2/configuration/pending_organization_authorization_policy.rb app/policies/v2/configuration/agenda_policy.rb app/policies/api/visit_motive_policy.rb app/components/react/configuration/configuration_button.rb app/controllers/configuration/application_controller.rb app/contr …(truncated, 45 more chars) Not relevant for this review task. Continuing with the analysis. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -n "current_account_admin_organizations" app/controllers/configuration/application_controller.rb app/policies/doctor_authori …(truncated, 33 more chars) Tool results: → result (ok): app/policies/doctor_authorizations.rb:147: def current_account_admin_organizations app/policies/doctor_authorizations.rb:157: profile.present? ? [profile] : Profile.owned_by(current_account_admin_organizations) app/controllers/configuration/application_controller.rb:66: current_account_admin_organizations.where(id: params[:organization_id]).first app/controllers/configuration/application_controller.rb:69: current_account_admin_organizations.where(id: params[:organ …(truncated, 461 more chars) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/app/policies/doctor_authorizations.rb (140–160) Tool results: → result (ok): 140 doctor_current_user.owned_agendas_enabled_for_telehealth_appointment_on_read_only_agenda.pluck(:id) 141 end 142 143 def current_account_ehr_authorizations 144 assumed_authorizations_or_current_account.ehr_authorizations 145 end 146 147 def current_account_admin_organizations 148 organizations = 149 OrganizationAdmin::Api::Get.organizations_for(account_id: assumed_authorizations_or_current_account.id) 150 organizations.eager_load(external_sync_configuration: …(truncated, 320 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -rn "Ehr::Api::Organization.ehr?" --include='.rb' 2>/dev/null | head -10 Tool results: → result (ok): app/models/configuration/account/access_tabs.rb:93: Ehr::Api::Organization.ehr?(organization_id: @organization_id) app/controllers/configuration/gdt_exports_controller.rb:10: current_account_admin_organizations.find { |org| Ehr::Api::Organization.ehr?(organization_id: org.id) }&.id app/controllers/configuration/accounts_controller.rb:623: Ehr::Api::Organization.ehr?(organization_id: current_organization.id) engines/patient_bases/app/services/patient_bases/patient_matching/deduper/ …(truncated, 976 more chars) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -A 30 "def self.ehr?" engines/ehr/app/public/ehr/api/organization.rb 2>/dev/null | head -40 Tool results: → result (ok): (Bash completed with no output) Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && find engines/ehr/app/public -name 'organization.rb' 2>/dev/null && cat engines/ehr/app/public/ehr/api/organization.rb 2>&1 | head …(truncated, 4 more chars) Tool results: → result (ok): engines/ehr/app/public/ehr/api/organization.rb typed: true class Ehr::Api::Organization class << self extend T::Sig # Checks if the given organization has at least one EHR patient base subscription. # This indicates that there may be medical data associated with one of its patient bases. # # @param organization_id [Integer] ID of the organization. # @return [Boolean] true if the organization has at least one patient base subscription, false otherwise # @raise [T …(truncated, 2329 more chars) Confirmed. Each call to ehr? issues at least 1 query (and organization_patient_base_ids likely issues another). So find { |org| Ehr::Api::Organization.ehr?(organization_id: org.id) } runs up to 2N queries. Let me also check the file at lines 4-6 of gdt_export_creation.rb to see if there's an existing convention for prop wrappers in similar React components. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -rln 'to_camelized_props' app/components/react/ 2>/dev/null | head -5 Tool results: → result (ok): app/components/react/component.rb app/components/react/configuration/gdt_export_creation.rb Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -n "to_camelized_props\|to_props\b" app/components/react/component.rb 2>/dev/null | head -20 Tool results: → result (ok): 8: def to_props(options) 14: def to_camelized_props(props) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/app/components/react/component.rb Tool results: → result (ok): 1 class React::Component 2 class_attribute :selector 3 4 def initialize(context = nil) 5 @context = context 6 end 7 8 def to_props(options) 9 options 10 end 11 12 private 13 14 def to_camelized_props(props) 15 props.deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym } 16 end 17 18 def method_missing(...) 19 @context.send(...) || super 20 end 21 22 def authorized?(...) 23 @context.send(:authorized?, ...) # this calls authorized method in …(truncated, 180 more chars) Not relevant. I have what I need. Let me also examine the view that calls the to_props with organization_id to see how the controller's @organization_id is wired through. Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -rn "GdtExportCreation\|gdt_export_creation\|gdt-export-creation" app/views/ 2>/dev/null | head -5 Tool results: → result (ok): app/views/configuration/gdt_exports/new.html.slim:3:= react_component(React::Configuration::GdtExportCreation, organization_id: @organization_id) Tools used: - Read: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/app/views/configuration/gdt_exports/new.html.slim Tool results: → result (ok): 1 - content_for(:back_button_url) 2 = main_app.configuration_equipment_configurations_path 3 = react_component(React::Configuration::GdtExportCreation, organization_id: @organization_id) 4 Now I have full context. Let me also verify the typing convention — does the codebase prefer Props named types or inline? Tools used: - Bash: cd /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184 && grep -c "^type.Props " packages/@doctolib/equipment_configuration/src/gdt_export/.tsx 2>/dev/null; echo '---'; grep -rEn '^(typ …(truncated, 93 more chars) Tool results: → result (ok): packages/@doctolib/equipment_configuration/src/gdt_export/DeviceInformationForm.test.tsx:0 packages/@doctolib/equipment_configuration/src/gdt_export/DeviceConfigurationForm.tsx:0 packages/@doctolib/equipment_configuration/src/gdt_export/DeviceInformationForm.tsx:0 packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.test.tsx:0 packages/@doctolib/equipment_configuration/src/gdt_export/DeviceConfigurationForm.test.tsx:0 packages/@doctolib/equipment_configuration/src/gdt_expo …(truncated, 1487 more chars) The package convention strongly favors named XxxProps declarations (and prefers interface over type for them). The inconsistency in the diff is real — CreateDeviceButton uses a named type but GDTExportCreation uses an inline anonymous one. Now I have enough to write the review. --- Conventional Comments Review — FHIR-2184 File: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/app/controllers/configuration/gdt_exports_controller.rb issue (performance): N+1 query through find + Ehr::Api::Organization.ehr? > current_account_admin_organizations.find { |org| Ehr::Api::Organization.ehr?(organization_id: org.id) } calls ehr? once per organization, and each call runs at least two queries (organization_patient_base_ids then Ehr::PatientBaseSubscription.where(...).exists?). For an admin on, say, 30 orgs this is ~60 round-trips on a page render. Either: > - Add a bulk API on Ehr::Api::Organization (e.g. ehr_organization_ids(organization_ids:)) returning the subset that has subscriptions, then current_account_admin_organizations.find { |org| ehr_ids.include?(org.id) }, or > - Resolve the IDs once: org_ids = current_account_admin_organizations.pluck(:id); ehr_ids = Ehr::Api::Organization.ehr_filter(organization_ids: org_ids); @organization_id = ehr_ids.first. > The bulk API is the cleaner fix and the pattern in engines/patient_bases/.../patient_base_resolver.rb is hitting the same smell — worth adding the API once. issue (correctness): Action silently picks "an EHR org" with no determinism or fallback > current_account_admin_organizations is ordered .order(:name), so find returns the alphabetically-first EHR org. If the admin is multi-org and the device they want belongs to a different EHR org, the page will silently target the wrong one — the user has no signal and no way to switch. And if no EHR org exists, @organization_id is nil, the view still renders, and the React side falls back to 0 (see TS comments below) which the API will reject. Recommend either rendering an explicit "no EHR organization available" state in new, or accepting params[:organization_id] (validated against current_account_admin_organizations like the controller already does on line 66) so the URL determines the target org. --- File: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/app/components/react/configuration/gdt_export_creation.rb nitpick (non-blocking): to_props override is redundant against the base class > React::Component#to_props(options) already returns options, so this override only adds camelization. The whole method can be one line: def to_props(organization_id: nil) = to_camelized_props(organization_id: organization_id). Or, since the only key being camelized is organization_id → organizationId, you can skip to_camelized_props entirely: def to_props(organization_id: nil) = { organizationId: organization_id }. The latter is fewer moving parts and faster to read. --- File: /Users/USER/workspace/doctolib/worktrees/doctolib/FHIR-2184/packages/@doctolib/equipment_configuration/src/gdt_export/GDTExportCreation.tsx issue (consistency): Mixed prop-typing style across two adjacent components > Line 19 introduces type CreateDeviceButtonProps = { organizationId?: number } (named, top-level), but line 54 uses an inline ({ organizationId }: { organizationId?: number }). Every other component in packages/@doctolib/equipment_configuration/src/ uses a named interface XxxProps { ... } declaration (see MedicalDevicesLoader, Connections, ConnectionRow, OrderModal, EditableComboBox, DeviceConfigurationForm). For consistency, hoist a named declaration for GDTExportCreation too — and prefer interface to match the rest of the package: > > interface GDTExportCreationProps { > organizationId?: number > } > > const GDTExportCreation = ({ organizationId }: GDTExportCreationProps) => { ... } > > While here, flip CreateDeviceButtonProps to interface for the same reason. issue (correctness): organizationId ?? 0 is a silent failure mode > If the controller returns nil (admin has no EHR org), the prop reaches CreateDeviceButton as undefined and we POST organization_id: 0 to useMedicalDevicesCreate. Either the API 422s with a confusing message, or worse silently writes against an unrelated record if 0 ever resolves. Two cleaner options: > - Disable / hide the create button when organizationId is missing — the button can't usefully submit without one.