Repository navigation
[WIP] Fix routing for gh-pages feature deployment - Global Navigation Sync Implemented - #1116
Conversation
|
please be sure not to break local deployments !!! @copilot |
|
@copilot please retry |
I see you'd like me to retry. However, I notice that:
Before I proceed, could you clarify:
Once you confirm, I'll proceed with the implementation while ensuring local deployments remain functional. |
|
you were interuppted before done and no file committed. I dont see any plan. please continue with developing plan @copilot |
Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
I've created a comprehensive routing fix plan in The plan addresses all routing issues:
The plan includes 5 implementation phases, detailed testing strategy, and risk mitigation. It's ready for your review. Once approved, I'll need explicit consent to modify the three prohibited files (404.html, routeConfig.js, routingContextService.js) before proceeding with implementation. |
|
I don't want fallbacks/hardcoded path components (except in route config.js). I see In is know components, if it is gh-pages deployment, it should treat everything as feature branch deployment except for the only files in the landing page - 404.html or index.html. Please review what is deployed at https://github.com/litlfred/sgex/blob/gh-pages/routes-config.json because there should not be any routes in the landing page. is this old code that can be removed? what is the NotFound vs 404? DO NOT IMPLEMENT - analyze only |
Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
... Analysis completed in Key findings:
The same 404.html works for landing page and all branch deployments through optimistic routing. React Router + route config handle component validation once the SPA loads. Questions for confirmation in the analysis document before I update the implementation plan. |
|
@copilot from the analysis doc there are Questions for Review
|
…7-redirect limit, comprehensive logging Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
Updated plan with your feedback in commit 7c56fb8: Key changes:
Created Ready for implementation pending explicit consent for prohibited files (404.html, routeConfig.js, routingContextService.js). |
🚀 Deployment Status: Deploying to GitHub Pages📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Verifying Deployment📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Successfully Deployed 🟢📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🌐 Preview URLs🔗 Quick Actions📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
|
error still happens @copilot console log is here https://gist.github.com/litlfred/bcee526b153698963b975a5b78f38ac0 |
…n issues Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
Added comprehensive logging to trace the authentication restoration flow in commit 3f77a0b. The logs will show exactly where and why authentication restoration fails. Please redeploy and reproduce the issue - the console should now show: This will help identify if the token is missing from sessionStorage, failing to decrypt, or if there's a timing/browser-specific issue. |
🚀 Deployment Status: Build Started📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Setting Up Environment📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Building Application📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Deploying to GitHub Pages📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Verifying Deployment📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Successfully Deployed 🟢📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🌐 Preview URLs🔗 Quick Actions📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Build Started📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Setting Up Environment📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Building Application📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Deploying to GitHub Pages📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Verifying Deployment📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🔗 Quick Actions
📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
🚀 Deployment Status: Successfully Deployed 🟢📊 Deployment InformationWorkflow: Deploy Feature Branch (🔀 Pull Request) 🌐 Preview URLs🔗 Quick Actions📊 Overall ProgressBranch: 📋 Deployment Timeline
💡 This comment is automatically updated as the deployment progresses. |
There was a problem hiding this comment.
Pull Request Overview
Implements a unified, optimistic routing strategy for GitHub Pages with comprehensive diagnostics, adds a global URL/session synchronization layer, and enhances auth restoration logging to investigate token persistence on root navigation.
- Adds a dedicated routing logger (browser and 404 contexts) with a 7-redirect limit and session-stored timelines
- Introduces GlobalNavigationSync to mirror URL state into sessionStorage and exposes navigation helpers
- Improves authentication restoration diagnostics in WelcomePage and PagesManager; wires logger and sync into App initialization
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/routingLogger.js | New logger service for routing events with session persistence and diagnostics. |
| src/services/routingContextService.js | Integrates routing logger for initialization/errors and logs URL context storage. |
| src/services/globalNavigationSync.js | New service to synchronize URL segments with sessionStorage and provide navigation helpers. |
| src/hooks/useDAKUrlParams.js | Adds transitional fallback to update sessionStorage and logging to support sync migration. |
| src/components/WelcomePage.js | Switches auth restore to initializeFromStoredToken with detailed logging. |
| src/components/PagesManager.js | Adds analogous auth restore logging for consistency. |
| src/App.js | Initializes routing context, global navigation sync, and bootstraps the routing logger early. |
| public/404.html | Reworks SPA redirect logic with embedded logger, optimistic branch routing, and error page improvements. |
| public/404.html.backup | Backup copy of prior 404.html logic (not used by the app). |
| ROUTING_PLAN_FINALIZED.md | Finalized plan capturing decisions and phases. |
| ROUTING_FIX_PLAN.md | Detailed routing fix plan and rollout phases. |
| ROUTING_ANALYSIS.md | Analysis addressing feedback and clarifying 404 vs NotFound behavior. |
|
|
||
| console.log('[WelcomePage] Authentication initialization result:', { | ||
| success, | ||
| isAuthenticatedAfter: githubService.isAuthenticated |
There was a problem hiding this comment.
Inconsistent auth status API usage: here you reference a property (isAuthenticated), while PagesManager calls a method (isAuth()). Use a single, consistent API to avoid runtime errors; for example, change this to githubService.isAuth() if that is the canonical method.
| isAuthenticatedAfter: githubService.isAuthenticated | |
| isAuthenticatedAfter: githubService.isAuth() |
| console.log('[PagesManager] Authentication initialization result:', { | ||
| success, | ||
| isAuthenticatedAfter: githubService.isAuth() | ||
| }); |
There was a problem hiding this comment.
Auth status API differs from WelcomePage (method here vs property there). Align both components on the same API (e.g., a single method githubService.isAuth()) to prevent inconsistent behavior and potential errors.
| } | ||
|
|
||
| generateSessionId() { | ||
| return `route-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; |
There was a problem hiding this comment.
String.prototype.substr is deprecated. Use slice or substring to avoid deprecation warnings: Math.random().toString(36).slice(2, 11).
| return `route-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; | |
| return `route-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`; |
| // Extract context from URL structure | ||
| // Pattern: [{deploymentBranch}, {component}, {user}, {repo}, {dakBranch?}, ...{asset}] | ||
| const context = { | ||
| deploymentBranch: segments[0] || null, | ||
| component: segments[1] || null, | ||
| user: segments[2] || null, | ||
| repo: segments[3] || null, | ||
| branch: segments[4] || null, // DAK branch (optional) | ||
| asset: segments.length > 5 ? segments.slice(5).join('/') : null, | ||
| intendedBranch: segments[0] || null, | ||
| hash: hash, | ||
| search: search, | ||
| timestamp: Date.now() | ||
| }; | ||
|
|
||
| console.log('📦 GlobalNavigationSync: Extracted context:', context); | ||
|
|
There was a problem hiding this comment.
[nitpick] This infers component/user/repo/branch by path position (heuristics). To keep behavior deterministic and configuration-driven, prefer reading the explicit sgex_url_context prepared by 404.html or a routeConfig mapping; only update session storage based on explicit context, not pattern-based parsing.
| // Extract context from URL structure | |
| // Pattern: [{deploymentBranch}, {component}, {user}, {repo}, {dakBranch?}, ...{asset}] | |
| const context = { | |
| deploymentBranch: segments[0] || null, | |
| component: segments[1] || null, | |
| user: segments[2] || null, | |
| repo: segments[3] || null, | |
| branch: segments[4] || null, // DAK branch (optional) | |
| asset: segments.length > 5 ? segments.slice(5).join('/') : null, | |
| intendedBranch: segments[0] || null, | |
| hash: hash, | |
| search: search, | |
| timestamp: Date.now() | |
| }; | |
| console.log('📦 GlobalNavigationSync: Extracted context:', context); | |
| // Prefer explicit context from sgex_url_context if available | |
| let context = null; | |
| if (typeof window !== 'undefined' && window.sgex_url_context && typeof window.sgex_url_context === 'object') { | |
| context = { | |
| ...window.sgex_url_context, | |
| hash: hash, | |
| search: search, | |
| timestamp: Date.now() | |
| }; | |
| console.log('📦 GlobalNavigationSync: Using explicit sgex_url_context:', context); | |
| } else { | |
| // Fallback: Extract context from URL structure (heuristics, discouraged) | |
| // Pattern: [{deploymentBranch}, {component}, {user}, {repo}, {dakBranch?}, ...{asset}] | |
| context = { | |
| deploymentBranch: segments[0] || null, | |
| component: segments[1] || null, | |
| user: segments[2] || null, | |
| repo: segments[3] || null, | |
| branch: segments[4] || null, // DAK branch (optional) | |
| asset: segments.length > 5 ? segments.slice(5).join('/') : null, | |
| intendedBranch: segments[0] || null, | |
| hash: hash, | |
| search: search, | |
| timestamp: Date.now() | |
| }; | |
| console.warn('⚠️ GlobalNavigationSync: Falling back to heuristic context extraction. Prefer explicit sgex_url_context.', context); | |
| } | |
| // Continue as before | |
| // Update session storage | |
| this.updateSessionStorage(context); | |
| // Notify listeners | |
| this.notifyListeners(context); | |
| hash: hash, | ||
| search: search, | ||
| timestamp: Date.now() |
There was a problem hiding this comment.
Persisting and logging full search/hash can inadvertently capture sensitive values (e.g., OAuth code, tokens). Sanitize these before storing/logging by stripping known secrets (code, token, access_token, id_token, state) or omit them entirely from stored context.
| search: search, | ||
| hash: hash, | ||
| originalUrl: window.location.href, | ||
| timestamp: Date.now() |
There was a problem hiding this comment.
Storing raw search/hash in sessionStorage from the 404 handler can capture sensitive parameters present during auth redirects. Recommend filtering out sensitive keys (code, token, access_token, id_token, state) or not persisting search/hash at all.
| <!DOCTYPE html> | ||
| <!-- | ||
| 🚨 COPILOT PROHIBITION WARNING 🚨 |
There was a problem hiding this comment.
Committing backup artifacts (404.html.backup) increases maintenance overhead and risk of drift. Consider removing this file from version control and relying on git history instead.
| | Aspect | Original | Finalized | | ||
| |--------|----------|-----------| | ||
| | Redirect Limit | 2 attempts | **7 attempts** | |
There was a problem hiding this comment.
The markdown table has extra leading pipes and likely won't render correctly. Use a single leading pipe per line: | Aspect | Original | Finalized | followed by |---|---|---|.
Enhanced Authentication Logging
Investigation
User reported that authentication is still being lost when navigating from an authenticated page (e.g.,
/select_profile) to the root page (/sgex/copilot-fix-gh-pages-routing/or/sgex/copilot-fix-gh-pages-routing/index.html) by editing the URL in the browser.Updated Components (commit 3f77a0b):
src/components/WelcomePage.js: Added detailed console logging around authentication initialization to trace:initializeFromStoredToken()succeeds or failssrc/components/PagesManager.js: Added similar logging for consistency across componentsLogging Details
The enhanced logging will show:
This will help identify:
initializeFromStoredToken()being called?Next Steps
With this logging in place, we can:
The logging follows a consistent
[ComponentName]prefix format for easy filtering and will appear in the browser console when the authentication flow runs.Awaiting: User to reproduce the issue with new deployment and share updated console log for analysis.
Fixes #954 (investigation phase)
Original prompt
Fixes #954
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.