fix(headless): incorrect last navigated URL#6278
Conversation
Signed-off-by: Dwi Siswanto <git@dw1.io>
for `ActionNavigate`, `WaitPageLifecycleEvent`, and `WaitStable` based on latest navigation URL. Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughThe changes introduce a new mechanism in the headless protocol engine to track and update the last navigated URL after navigation actions. This includes adding a field to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page
participant Instance
User->>Page: Trigger navigation action
Page->>Page: Set lastActionNavigate
Page->>Page: updateLastNavigatedURL()
Page->>Instance: Update requestLog with final navigated URL
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/protocols/headless/engine/page.go (2)
279-288: Consider thread safety for the new method.The
updateLastNavigatedURL()method correctly updates the request log mapping. However, consider thread safety concerns since the Page struct has a mutex but this method doesn't use it when accessingp.instance.requestLog.If
requestLogcan be accessed concurrently, consider adding synchronization:func (p *Page) updateLastNavigatedURL() { if p.lastActionNavigate == nil { return } + p.mutex.Lock() + defer p.mutex.Unlock() + templateURL := p.lastActionNavigate.GetArg("url") p.instance.requestLog[templateURL] = p.URL() }
201-201: Remove commented debug code.The commented debug print statement should be removed or properly implemented as conditional debug logging.
- // fmt.Printf("firstItem.RawResponse: %v\n", firstItem.RawResponse)pkg/protocols/headless/engine/page_actions.go (1)
674-674: Consider potential redundancy in URL state updates.While updating the URL state after lifecycle events is appropriate, consider if this might result in redundant updates when multiple navigation-related actions are chained together.
The current approach ensures correctness but may result in multiple updates. If performance is a concern, consider debouncing or checking if an update is actually needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/protocols/headless/engine/page.go(3 hunks)pkg/protocols/headless/engine/page_actions.go(5 hunks)pkg/protocols/headless/request.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint
🔇 Additional comments (6)
pkg/protocols/headless/request.go (2)
193-194: LGTM! Map copying refactored to use standard library.The refactoring from manual map iteration to
maps.Copyis cleaner and more efficient. The behavior remains functionally equivalent while reducing code complexity.
5-5: ```shell
#!/bin/bashLocate go.mod and extract Go version requirement
find . -maxdepth 2 -name 'go.mod' -print0 | while IFS= read -r -d '' file; do
echo "File: $file"
grep '^go ' "$file" || echo "No Go version specified in $file"
done</details> <details> <summary>pkg/protocols/headless/engine/page.go (1)</summary> `40-40`: **LGTM! New field for tracking navigation state.** The `lastActionNavigate` field appropriately tracks the last navigation action for URL state management. </details> <details> <summary>pkg/protocols/headless/engine/page_actions.go (3)</summary> `81-81`: **LGTM! Correct placement of navigation state tracking.** Setting `p.lastActionNavigate = act` after successful navigation ensures the state is only updated when navigation succeeds. --- `417-417`: **LGTM! URL state updated after navigation.** Calling `updateLastNavigatedURL()` immediately after navigation ensures the request log reflects the current page state. --- `701-701`: **LGTM! URL state updated after page stabilization.** Updating the URL state after `WaitStable` is appropriate since page redirects can occur during stabilization. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
ehsandeep
deleted the
dwisiswant0/fix/headless/incorrect-last-navigated-URL
branch
3 participants
Proposed changes
Fixes #6274
Checklist
Summary by CodeRabbit