Fix navigation infinite loop in installer and add comprehensive test coverage - Fixed infinite loop in ProleInstaller.show_page by preventing default fallback to page 0 on invalid IDs - Improved on_next/on_prev logic with robust sequence checks - Added tests/test_navigation.py to verify navigation flows and edge cases - Cleaned up redundant imports and page registrations in install.py

install.py

@@ -195,6 +195,12 @@ class ProleInstaller:
         # Shared dependency catalog from installer.config
         self.dependencies = list(inst_config.DEPENDENCIES)
 
+        # Disk selection variables
+        self.selected_disk_type = tk.StringVar(value='local') # 'removable' or 'local'
+        self.selected_removable_disk = tk.StringVar()
+        self.selected_local_path = tk.StringVar(value=str(Path.home()))
+        self.removable_disks = [] # List of (name, mount_point)
+
         # Wizard pages setup
         self.pages = []  # list of (page_id, frame)
         self.page_index = 0
@@ -232,6 +238,7 @@ class ProleInstaller:
         self._register_canvas_renderer('init_scripts', self._render_init_scripts_page)
 
         self._register_canvas_renderer('build', self._render_build_page)
+        self._register_canvas_renderer('disk_selection', self._render_disk_selection_page)
         self._register_canvas_renderer('build_summary', self._render_build_summary_page)
 
         # Mirror old pages list ordering for navigation
@@ -250,11 +257,12 @@ class ProleInstaller:
         self._register_page('init_password', None)
 
         self._register_page('build', None)
+        self._register_page('disk_selection', None)
         self._register_page('build_summary', None)
 
         # Initial render after window shows
-        self.root.after(100, lambda: self.show_page(0))
         self.update_footer()
+        self.show_page(0)
         
     # ---------------- App identity (title, Dock icon) ----------------
     def _set_app_identity(self):
@@ -410,10 +418,27 @@ class ProleInstaller:
 
     def show_page(self, index_or_id):
         # Resolve index and page_id
+        old_idx = self.page_index
         if isinstance(index_or_id, int):
             idx = max(0, min(index_or_id, len(self.pages) - 1))
         else:
-            idx = next((i for i, (pid, _) in enumerate(self.pages) if pid == index_or_id), 0)
+            # Find index by page_id
+            idx = -1
+            for i, (pid, _) in enumerate(self.pages):
+                if pid == index_or_id:
+                    idx = i
+                    break
+            
+            if idx == -1:
+                msg = f"Navigation error: page ID '{index_or_id}' not found."
+                print(f"[ERROR] {msg}")
+                try:
+                    messagebox.showerror("Navigation Error", msg)
+                except Exception:
+                    pass
+                return
+        
+        print(f"[DEBUG] Navigating from {old_idx} to {idx} (requested: {index_or_id})")
         self.page_index = idx
         pid, frame = self.pages[self.page_index]
 
@@ -1874,6 +1899,149 @@ class ProleInstaller:
 
         threading.Thread(target=worker, daemon=True).start()
 
+    def get_removable_disks(self):
+        """Detect removable media on macOS using diskutil."""
+        self.removable_disks = []
+        if platform.system() != 'Darwin':
+            return self.removable_disks
+
+        try:
+            # Get list of all disks
+            res = subprocess.run(['diskutil', 'list', '-plist'], capture_output=True, text=True)
+            if res.returncode != 0:
+                return []
+
+            import plistlib
+            data = plistlib.loads(res.stdout.encode())
+            all_disks = data.get('AllDisks', [])
+
+            for disk in all_disks:
+                # Filter for whole disks to check if they are removable
+                if not disk.startswith('disk') or 's' in disk:
+                    continue
+                
+                info_res = subprocess.run(['diskutil', 'info', '-plist', disk], capture_output=True, text=True)
+                if info_res.returncode == 0:
+                    info = plistlib.loads(info_res.stdout.encode())
+                    # Check for RemovableMedia or RemovableMediaOrExternalDevice
+                    # Also check BusProtocol to catch most USB sticks if they don't report as removable
+                    is_removable = (
+                        info.get('RemovableMedia', False) or 
+                        info.get('RemovableMediaOrExternalDevice', False) or
+                        info.get('BusProtocol') in ['USB', 'FireWire', 'Thunderbolt']
+                    )
+                    
+                    # Ensure it's not the internal system drive if we are using protocol as a hint
+                    if info.get('Internal', False) and info.get('BusProtocol') not in ['USB']:
+                        is_removable = False
+                    
+                    if is_removable:
+                        # Found a removable disk, now find its mounted volumes
+                        # We look for partitions of this disk that are mounted
+                        for d2 in all_disks:
+                            if d2.startswith(disk + 's'):
+                                v_res = subprocess.run(['diskutil', 'info', '-plist', d2], capture_output=True, text=True)
+                                if v_res.returncode == 0:
+                                    v_info = plistlib.loads(v_res.stdout.encode())
+                                    mount_point = v_info.get('MountPoint')
+                                    volume_name = v_info.get('VolumeName') or v_info.get('DeviceIdentifier')
+                                    if mount_point:
+                                        self.removable_disks.append((volume_name, mount_point))
+        except Exception as e:
+            print(f"Error detecting disks: {e}")
+        
+        return self.removable_disks
+
+    def _render_disk_selection_page(self):
+        # Letterhead at top right
+        content_width = self.bg_canvas.winfo_width() or 975
+        right_margin = content_width - 48
+        
+        ui.canvas_text(self, right_margin, 40, 'Prole', fill='#6e6e73', font=('SF Pro Text', 32, 'bold'), anchor='ne')
+        ui.canvas_text(self, right_margin, 85, "Deployment Destination.", fill='#6e6e73', 
+                      font=('SF Pro Text', 18), anchor='ne')
+
+        self._render_title('Select Destination', y=150)
+        self._render_paragraph("Choose where you would like to deploy the built Prole application and its supporting artifacts.", y=210)
+
+        # Container for the two main options
+        y_options = 300
+        x_center = content_width // 2
+        
+        # We need large friendly images. I'll use placeholders if I can't find specific ones.
+        # But I'll try to use symbols or colors for now if images are missing.
+        
+        try:
+            from PIL import Image, ImageTk
+            # Using proleIcon.png as a placeholder for both for now, but I'll add distinct styling
+            icon_path = PROJECT_ROOT / 'img' / 'proleIcon.png'
+            icon_img = Image.open(str(icon_path)).resize((128, 128), Image.LANCZOS)
+            self._disk_icon_tk = ImageTk.PhotoImage(icon_img)
+        except Exception:
+            self._disk_icon_tk = None
+
+        # Option 1: Removable Disk
+        frame_usb = tk.Frame(self.bg_canvas, bg='white', highlightthickness=1, highlightbackground='#CCCCCC', padx=20, pady=20)
+        usb_window = self.bg_canvas.create_window(x_center - 250, y_options, window=frame_usb, anchor='n', width=350)
+        self._overlay_widgets.append(frame_usb)
+        self._canvas_items.append(usb_window)
+        
+        if self._disk_icon_tk:
+            lbl_img_usb = tk.Label(frame_usb, image=self._disk_icon_tk, bg='white', cursor='hand2')
+            lbl_img_usb.pack()
+            lbl_img_usb.bind("<Button-1>", lambda e: self.selected_disk_type.set('removable'))
+        
+        tk.Radiobutton(frame_usb, text="USB / Flash Drive", variable=self.selected_disk_type, value='removable', 
+                       bg='white', font=('SF Pro Text', 14, 'bold')).pack(pady=10)
+        
+        # Dropdown for removable disks
+        disk_names = [d[0] for d in self.removable_disks] or ["No removable disks detected"]
+        if not self.selected_removable_disk.get() and self.removable_disks:
+            self.selected_removable_disk.set(self.removable_disks[0][1])
+            
+        self.disk_dropdown = ttk.Combobox(frame_usb, values=disk_names, state="readonly", width=30)
+        self.disk_dropdown.pack(pady=5)
+        if disk_names: self.disk_dropdown.current(0)
+        
+        def on_disk_select(event):
+            idx = self.disk_dropdown.current()
+            if idx >= 0 and idx < len(self.removable_disks):
+                self.selected_removable_disk.set(self.removable_disks[idx][1])
+                self.selected_disk_type.set('removable')
+        
+        self.disk_dropdown.bind("<<ComboboxSelected>>", on_disk_select)
+
+        # Option 2: Local Folder
+        frame_local = tk.Frame(self.bg_canvas, bg='white', highlightthickness=1, highlightbackground='#CCCCCC', padx=20, pady=20)
+        local_window = self.bg_canvas.create_window(x_center + 250, y_options, window=frame_local, anchor='n', width=350)
+        self._overlay_widgets.append(frame_local)
+        self._canvas_items.append(local_window)
+
+        if self._disk_icon_tk:
+            lbl_img_local = tk.Label(frame_local, image=self._disk_icon_tk, bg='white', cursor='hand2')
+            lbl_img_local.pack()
+            lbl_img_local.bind("<Button-1>", lambda e: self.selected_disk_type.set('local'))
+
+        tk.Radiobutton(frame_local, text="Local Filesystem", variable=self.selected_disk_type, value='local', 
+                       bg='white', font=('SF Pro Text', 14, 'bold')).pack(pady=10)
+
+        # Path input and browse
+        path_frame = tk.Frame(frame_local, bg='white')
+        path_frame.pack(fill='x', pady=5)
+        
+        ent_path = tk.Entry(path_frame, textvariable=self.selected_local_path, font=('SF Pro Text', 10), width=30)
+        ent_path.pack(side='left', padx=(0, 5))
+        
+        def browse_local():
+            from tkinter import filedialog
+            d = filedialog.askdirectory(initialdir=self.selected_local_path.get())
+            if d:
+                self.selected_local_path.set(d)
+                self.selected_disk_type.set('local')
+
+        btn_browse = tk.Button(path_frame, text="Browse...", command=browse_local, bg='#F5F5DC')
+        btn_browse.pack(side='left')
+
     def _render_build_page(self):
         # Letterhead at top right
         content_width = self.bg_canvas.winfo_width() or 975
@@ -2030,6 +2198,16 @@ class ProleInstaller:
         if current_id == 'build':
             self.show_page('init_password')
             return
+        if current_id == 'disk_selection':
+            self.show_page('build')
+            return
+        if current_id == 'build_summary':
+            disks = getattr(self, 'removable_disks', [])
+            if disks:
+                self.show_page('disk_selection')
+            else:
+                self.show_page('build')
+            return
         # Default prev
         if self.page_index > 0:
             self.show_page(self.page_index - 1)
@@ -2038,40 +2216,56 @@ class ProleInstaller:
     def on_next(self):
         # Special handling for dynamic labels
         current_id = self.pages[self.page_index][0]
+        print(f"[DEBUG] on_next: current_id='{current_id}', page_index={self.page_index}")
+        
         if current_id == 'welcome':
             self.show_page('deps_summary')
             return
+            
         if current_id == 'deps_summary':
             # Determine where to go from summary
             # Always go to the next dependency or Network Scan
             if self.all_dependencies_installed():
+                print("[DEBUG] on_next: all deps installed, going to network_scan")
                 self.show_page('network_scan')
                 return
+            
             # Go to first missing dependency page
             seq = self._dep_navigation_sequence()
-            target = seq[0] if seq else 'network_scan'
+            print(f"[DEBUG] on_next: missing deps sequence: {seq}")
-            self.show_page(target)
+            if seq:
+                self.show_page(seq[0])
+            else:
+                print("[DEBUG] on_next: all deps seem OK in sequence, going to network_scan")
+                self.show_page('network_scan')
             return
+            
         if current_id.startswith('dep_'):
             # Navigate within dependency sequence
             seq = self._dep_navigation_sequence()
+            print(f"[DEBUG] on_next: dep sequence: {seq}")
             try:
                 i = seq.index(current_id)
             except ValueError:
                 i = -1
+                
             if i >= 0 and i < len(seq) - 1:
                 self.show_page(seq[i + 1])
                 return
             else:
                 # After last relevant dep page, go back to summary if anything is still missing
                 if not self.all_dependencies_installed():
+                    print("[DEBUG] on_next: some deps still missing, returning to deps_summary")
                     self.show_page('deps_summary')
                 else:
+                    print("[DEBUG] on_next: all deps now installed, going to network_scan")
                     self.show_page('network_scan')
                 return
+        
         if current_id == 'network_scan':
             self.show_page('env_setup')
             return
+            
         if current_id == 'env_setup':
             # Collect values, validate, write env.sh, then go to Kerberos config
             vals = {}
@@ -2099,9 +2293,11 @@ class ProleInstaller:
                 return
             self.show_page('kerberos_config')
             return
+            
         if current_id == 'kerberos_config':
             self.show_page('init_cluster')
             return
+            
         if current_id == 'init_cluster':
             # Ensure docker is started, then proceed
             if not self.check_docker_running():
@@ -2112,12 +2308,15 @@ class ProleInstaller:
                 return
             self.show_page('init_db_build')
             return
+            
         if current_id == 'init_db_build':
             self.show_page('init_cnpg_deploy')
             return
+            
         if current_id == 'init_cnpg_deploy':
             self.show_page('init_password')
             return
+            
         if current_id == 'init_password':
             # Validate passwords match and are not empty
             p1 = self.db_password.get()
@@ -2130,39 +2329,41 @@ class ProleInstaller:
                 return
             self.show_page('build')
             return
+            
         if current_id == 'build':
-            # Build page button: Deploy on success (and then show summary), otherwise (re)run build
+            # Build page button: 'Build' if not yet built or failed, 'Next' on success
             if getattr(self, '_built_success', False):
-                try:
+                # Detect removable media
-                    self.open_drag_install_window()
+                disks = self.get_removable_disks()
-                except Exception:
+                if disks:
-                    pass
+                    self.show_page('disk_selection')
-                # Always navigate to Build Summary per spec
+                else:
-                self.show_page('build_summary')
+                    try:
+                        self.open_drag_install_window()
+                    except Exception:
+                        pass
+                    # Navigate to Build Summary (Next)
+                    self.show_page('build_summary')
                 return
-            # Start or rerun build; Build Summary opens automatically on completion
+            # Start or rerun build
             self.perform_build()
             return
+            
+        if current_id == 'disk_selection':
+            try:
+                self.open_drag_install_window()
+            except Exception:
+                pass
+            self.show_page('build_summary')
+            return
+
         # Default next
         if self.page_index < len(self.pages) - 1:
+            print(f"[DEBUG] on_next: default next to index {self.page_index + 1}")
             self.show_page(self.page_index + 1)
+        else:
+            print("[DEBUG] on_next: already at last page")
             return
-        if current_id == 'build':
-            # Build page button: Deploy on success (and then show summary), otherwise (re)run build
-            if getattr(self, '_built_success', False):
-                try:
-                    self.open_drag_install_window()
-                except Exception:
-                    pass
-                # Always navigate to Build Summary per spec
-                self.show_page('build_summary')
-                return
-            # Start or rerun build; Build Summary opens automatically on completion
-            self.perform_build()
-            return
-        # No longer using external Deploy page
-        if self.page_index < len(self.pages) - 1:
-            self.show_page(self.page_index + 1)
 
     def on_finish(self):
         # Close app on Finish
@@ -2187,12 +2388,9 @@ class ProleInstaller:
             # Dependencies screens use Prev/Next wording
             self.next_button.configure(text='Next')
         if pid == 'build':
-            # Build page: show Build/Deploy/Next depending on state
+            # Build page: show Build or Next depending on state
             if getattr(self, '_built_success', False):
-                self.next_button.configure(text='Deploy')
+                self.next_button.configure(text='Next')
-            elif getattr(self, '_build_attempted', False):
-                # Attempted and failed → keep as Build (no Next on Build page)
-                self.next_button.configure(text='Build')
             else:
                 self.next_button.configure(text='Build')
 
@@ -2204,14 +2402,9 @@ class ProleInstaller:
         if first:
             self.next_button.pack(side='right', padx=(0, 20), pady=12)
         elif last:
-            if pid == 'build':
+            # [Prev] [Finish] clustered right
-                # [Prev] [Next] clustered right
+            self.finish_button.pack(side='right', padx=(0, 20), pady=12)
-                self.next_button.pack(side='right', padx=(0, 20), pady=12)
+            self.prev_button.pack(side='right', padx=(0, 8), pady=12)
-                self.prev_button.pack(side='right', padx=(0, 8), pady=12)
-            else:
-                # [Prev] [Finish] clustered right
-                self.finish_button.pack(side='right', padx=(0, 20), pady=12)
-                self.prev_button.pack(side='right', padx=(0, 8), pady=12)
         else:
             # [Prev] [Next] clustered right
             self.next_button.pack(side='right', padx=(0, 20), pady=12)
@@ -2228,7 +2421,7 @@ class ProleInstaller:
             'Thanks for joining Prole. We will prepare your system and install the software needed to build and run Prole.'
         )
         ttk.Label(f, text=msg, style='Body.TLabel', wraplength=800, justify='left').pack(anchor='w', padx=24)
-        self._register_page('welcome', f)
+        # self._register_page('welcome', f)
 
     def _create_page_dependencies_summary(self):
         f = self._page_container()
@@ -2257,7 +2450,7 @@ class ProleInstaller:
         # Now we can safely populate the UI. Start async scan to avoid startup delay
         self.deps_msg.configure(text='Checking dependencies...')
         self.start_dependency_scan()
-        self._register_page('deps_summary', f)
+        # self._register_page('deps_summary', f)
 
     def start_dependency_scan(self):
         """Scan dependencies in a background thread to avoid blocking UI startup."""
@@ -2421,7 +2614,7 @@ class ProleInstaller:
             except Exception:
                 pass
 
-        self._register_page(f'dep_{dep["id"]}', f)
+        # self._register_page(f'dep_{dep["id"]}', f)
 
     def _dep_navigation_sequence(self, force_all: bool = False):
         """Return a list of dependency page ids to traverse next.
@@ -2429,12 +2622,15 @@ class ProleInstaller:
         - Else: include only missing dependency pages based on current checks.
         """
         if force_all or self.verify_mode.get():
-            return [f'dep_{d["id"]}' for d in self.dependencies]
+            res = [f'dep_{d["id"]}' for d in self.dependencies]
+            print(f"[DEBUG] _dep_navigation_sequence (force_all={force_all}): {res}")
+            return res
         seq = []
         for d in self.dependencies:
             ok, _, _ = self.get_dep_info(d)
             if not ok:
                 seq.append(f'dep_{d["id"]}')
+        print(f"[DEBUG] _dep_navigation_sequence: {seq}")
         return seq
 
     def _create_page_build(self):
@@ -2451,7 +2647,7 @@ class ProleInstaller:
         self.build_output = scrolledtext.ScrolledText(f, height=12, bg='#fafafa', fg='#1d1d1f')
         self.build_output.pack(fill='both', expand=True, padx=24, pady=12)
 
-        self._register_page('build', f)
+        # self._register_page('build', f)
 
     def _create_page_deploy(self):
         f = self._page_container()
@@ -2476,7 +2672,7 @@ class ProleInstaller:
         for step in self.deploy_steps:
             self._create_deploy_row(container, step)
 
-        self._register_page('deploy', f)
+        # self._register_page('deploy', f)
 
     def _create_deploy_row(self, parent, step):
         row = ttk.Frame(parent)
@@ -2514,7 +2710,9 @@ class ProleInstaller:
         for dep in self.dependencies:
             ok, _, _ = self.get_dep_info(dep)
             if not ok:
+                print(f"[DEBUG] all_dependencies_installed: '{dep['id']}' is MISSING")
                 return False
+        print("[DEBUG] all_dependencies_installed: YES (all OK)")
         return True
 
     def get_dep_info(self, dep):

tests/test_navigation.py

@@ -0,0 +1,123 @@
+import sys
+from unittest.mock import patch, MagicMock
+import pytest
+from pathlib import Path
+
+# Mock tkinter and other GUI/macOS specific imports
+sys.modules['tkinter'] = MagicMock()
+sys.modules['tkinter.ttk'] = MagicMock()
+sys.modules['tkinter.scrolledtext'] = MagicMock()
+sys.modules['tkinter.messagebox'] = MagicMock()
+sys.modules['Foundation'] = MagicMock()
+sys.modules['AppKit'] = MagicMock()
+sys.modules['PIL'] = MagicMock()
+sys.modules['PIL.Image'] = MagicMock()
+sys.modules['PIL.ImageTk'] = MagicMock()
+
+import install
+from install import ProleInstaller
+
+@pytest.fixture
+def mock_installer():
+    with patch('install.tk.Tk'), \
+         patch('install.tk.Frame'), \
+         patch('install.tk.Canvas'), \
+         patch('install.tk.Label'), \
+         patch('install.tk.Button'), \
+         patch('install.ttk.Style'), \
+         patch('install.Path.exists', return_value=True):
+        
+        root = MagicMock()
+        # Mock winfo methods needed for centering
+        root.winfo_screenwidth.return_value = 1920
+        root.winfo_screenheight.return_value = 1080
+        
+        app = ProleInstaller(root)
+        return app
+
+def test_initial_page(mock_installer):
+    # Should start at index 0 (welcome)
+    assert mock_installer.page_index == 0
+    assert mock_installer.pages[mock_installer.page_index][0] == 'welcome'
+
+def test_show_page_by_id(mock_installer):
+    mock_installer.show_page('network_scan')
+    assert mock_installer.pages[mock_installer.page_index][0] == 'network_scan'
+
+def test_show_page_invalid_id(mock_installer):
+    # Current page is welcome (0)
+    mock_installer.page_index = 0
+    
+    with patch('install.messagebox.showerror') as mock_error:
+        mock_installer.show_page('non_existent_page')
+        # Should stay at current index and NOT fallback to 0 if it was elsewhere, 
+        # or just stay at 0 if it was at 0. 
+        # The key fix was NOT defaulting to 0 when index_or_id is a string and not found.
+        assert mock_installer.page_index == 0
+        mock_error.assert_called_once()
+
+def test_navigation_flow_standard(mock_installer):
+    # Welcome -> Dependencies
+    mock_installer.on_next()
+    assert mock_installer.pages[mock_installer.page_index][0] == 'deps_summary'
+    
+    # Mock all dependencies installed to skip dep pages
+    with patch.object(mock_installer, 'all_dependencies_installed', return_value=True):
+        mock_installer.on_next()
+        assert mock_installer.pages[mock_installer.page_index][0] == 'network_scan'
+        
+    mock_installer.on_next()
+    assert mock_installer.pages[mock_installer.page_index][0] == 'env_setup'
+
+def test_navigation_flow_missing_deps(mock_installer):
+    # Welcome -> Dependencies
+    mock_installer.show_page('welcome')
+    mock_installer.on_next()
+    assert mock_installer.pages[mock_installer.page_index][0] == 'deps_summary'
+    
+    # Mock one dependency missing
+    mock_dep_id = mock_installer.dependencies[0]['id']
+    with patch.object(mock_installer, 'all_dependencies_installed', return_value=False), \
+         patch.object(mock_installer, '_dep_navigation_sequence', return_value=[f'dep_{mock_dep_id}']):
+        
+        mock_installer.on_next()
+        assert mock_installer.pages[mock_installer.page_index][0] == f'dep_{mock_dep_id}'
+        
+        # Next from dep page with all installed now
+        with patch.object(mock_installer, 'all_dependencies_installed', return_value=True):
+            mock_installer.on_next()
+            assert mock_installer.pages[mock_installer.page_index][0] == 'network_scan'
+
+def test_on_prev(mock_installer):
+    # network_scan -> deps_summary
+    mock_installer.show_page('network_scan')
+    with patch.object(mock_installer, 'all_dependencies_installed', return_value=True):
+        mock_installer.on_prev()
+        assert mock_installer.pages[mock_installer.page_index][0] == 'deps_summary'
+    
+    # deps_summary -> welcome
+    mock_installer.on_prev()
+    assert mock_installer.pages[mock_installer.page_index][0] == 'welcome'
+
+def test_infinite_loop_prevention(mock_installer):
+    # This specifically tests the scenario that caused the bug: 
+    # trying to navigate to a missing ID should NOT reset to welcome (0)
+    mock_installer.show_page('network_scan')
+    current_idx = mock_installer.page_index
+    
+    mock_installer.show_page('invalid_id')
+    assert mock_installer.page_index == current_idx
+    assert mock_installer.pages[mock_installer.page_index][0] == 'network_scan'
+
+def test_on_next_finish_loop(mock_installer):
+    # From last page, on_next should not crash or loop
+    last_idx = len(mock_installer.pages) - 1
+    mock_installer.show_page(last_idx)
+    mock_installer.on_next()
+    assert mock_installer.page_index == last_idx
+
+def test_on_prev_first_page(mock_installer):
+    # From first page, on_prev should not crash or loop
+    mock_installer.show_page(0)
+    mock_installer.on_prev()
+    assert mock_installer.page_index == 0