docs: tweak the redirect implementation in webpage#48
docs: tweak the redirect implementation in webpage#48snipsnipsnip merged 2 commits intoexteditor:mainfrom
Conversation
I've noticed that we can implement redirection without enabling the `executeScript` option.
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR refactors the documentation site's redirect mechanism from script-based to JSON-based, adds a new tryRedirect utility to parse and execute redirects, updates URL encoding for resource types, consolidates imports for options initialization, and converts the main options startup flow from Promise chains to async/await. Changes
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant Site as index.mjs
participant Router as OptionsEventRouter
participant Element as GhostbirdOptionsElement
rect rgb(200, 220, 255)
note over Site: Old: Script-based redirect
User->>Browser: Click link with redirectTo
Browser->>Site: beforeEach processes HTML script
Site->>Browser: Execute inline script
Browser->>Browser: location.replace() after script execution
end
rect rgb(220, 255, 220)
note over Site: New: JSON-based redirect
User->>Browser: Click link with redirectTo
Browser->>Site: beforeEach invokes tryRedirect(text)
Site->>Site: Parse JSON {"redirectTo": url}
Site->>Browser: location.replace(url) immediately
Browser->>Browser: Navigate to new location
end
rect rgb(255, 240, 200)
note over Router: Old: Promise.then() pattern
Element->>Router: Custom element defined
Router->>Router: .then(e => initOptions(e))
end
rect rgb(255, 220, 220)
note over Router: New: async/await pattern
Element->>Router: Custom element defined
Router->>Router: await runRouterAsync()
Router->>Router: await router.initOptions(element)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span three files with mixed complexity: straightforward type annotation updates, URL encoding logic modifications, and a moderate async/await refactoring pattern. The JSON-based redirect mechanism introduces new parsing logic that requires verification, while the options initialization consolidation follows a standard pattern conversion from callbacks to async/await. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/root/options.ts (1)
7-26: LGTM: Improved readability with async/await.The refactoring from Promise chains to async/await significantly improves code readability while maintaining equivalent functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/site/index.mjs(4 hunks)src/app-options/options_event_router.ts(1 hunks)src/root/options.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/app-options/options_event_router.ts (1)
src/app-options/ghostbird_options_element.ts (1)
GhostbirdOptionsElement(3-53)
src/root/options.ts (3)
src/app-options/options_event_router.ts (1)
OptionsEventRouter(3-11)src/root/startup/startup_options.ts (1)
startupOptions(18-19)src/app-options/ghostbird_options_element.ts (1)
GhostbirdOptionsElement(3-53)
🔇 Additional comments (8)
.github/site/index.mjs (4)
14-14: LGTM: Defensive encoding of resource type.Encoding the type parameter is a safe defensive practice, even though 'blob' and 'tree' don't require encoding.
67-67: LGTM: Security improvement by disabling script execution.Changing
executeScripttofalseprevents arbitrary script execution in markdown content, which is a valuable security hardening measure.
77-77: LGTM: LICENSE route addition.The new route handles the LICENSE file consistently with other file routes.
102-102: LGTM: Correct hook placement for redirect handling.The
tryRedirecthook is correctly positioned as the firstbeforeEachhook to intercept redirect instructions before other processing occurs.src/app-options/options_event_router.ts (1)
8-10: LGTM: Accurate type reflects non-resolving promise.The return type
Promise<never>correctly reflects thatstartSynccontains an infinite loop and never resolves normally.src/root/options.ts (3)
3-3: LGTM: Consolidated imports.The import consolidation from a central "src/app-options" module improves maintainability.
28-28: LGTM: More semantic logging level.Using
console.infofor startup messages is more semantically appropriate thanconsole.log.Also applies to: 34-34
30-30: Intentional non-resolving await at module level.The top-level
await mainPromisewaits on aPromise<never>that runs an infinite event loop, meaning this module never completes loading. This is acceptable for an entrypoint script but would cause issues if this module were ever dynamically imported elsewhere.Per the PR description, this design is intentional.
Also applies to: 36-36
Some refactoring.
enabling the
$docsify.executeScriptoption.promise, even though it is not expected to resolve.