feat: force polling for t8 #184
Conversation
WalkthroughThe changes introduce several enhancements across multiple files. A new method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TrackingLiveUpdater
participant TrackingActor
participant TrackingMachine
User->>TrackingLiveUpdater: Click Refresh Button
TrackingLiveUpdater->>TrackingActor: Send forcePoll action
TrackingActor->>TrackingMachine: Trigger forcePoll event
TrackingMachine->>CommandHandler: Call ForcePoll()
CommandHandler->>Tracker: Execute polling logic
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 9
🧹 Outside diff range and nitpick comments (9)
pkg/tracker/tracker.go (1)
12-12: LGTM! Consider adding a comment for the new method.The addition of the
ForcePoll()method to theGameTrackerinterface is appropriate and aligns with the PR objectives. It provides a clear way to trigger polling on demand.Consider adding a brief comment to describe the purpose of the
ForcePoll()method, for example:// ForcePoll triggers an immediate poll action, regardless of the regular polling interval. ForcePoll()This would improve the documentation and make the interface more self-explanatory for future developers.
gui/src/state/tracking-machine.ts (3)
33-35: Consider adding error handling to theforcePollaction.The new
forcePollaction is correctly implemented and placed within theactionsobject. However, it might be beneficial to add error handling to theForcePollfunction call.Consider updating the action to include error handling:
forcePoll: ({ self }) => { ForcePoll().catch(error => self.send({ type: 'error', error })) },This change would make the error handling consistent with other actions in the file, such as
startTrackingandstopTracking.
96-98: LGTM: New event handler added correctly.The
forcePollevent handler is correctly implemented within thetrackingstate and triggers theforcePollaction when the event is received.Consider adding a guard to ensure that force polling is only triggered when appropriate. For example:
forcePoll: { guard: 'canForcePoll', actions: ['forcePoll'] },You would need to implement the
canForcePollguard in theguardssection of the machine setup. This could prevent unnecessary polling and improve the overall efficiency of the application.
Line range hint
1-124: Summary: Force polling feature implemented correctly with room for minor improvements.The changes to implement the force polling feature are well-structured and integrate seamlessly with the existing state machine. The new import, action, and event handler are all correctly implemented.
However, there are a few areas where the implementation could be further improved:
- Adding error handling to the
forcePollaction.- Implementing a guard for the
forcePollevent to ensure it's only triggered when appropriate.These improvements would enhance the robustness and efficiency of the force polling feature.
Consider implementing a rate limiting mechanism for the force poll action to prevent potential abuse or excessive server load. This could be done by adding a cooldown period after each force poll, implemented either in the state machine or at the API level.
pkg/tracker/sfv/track.go (1)
103-104: Consider integratingForcePollwith the existing polling mechanism.The
ForcePollmethod is currently isolated from the rest of the tracking logic. Consider how it should interact with the existingpollmethod and theisTrackingstate.Some considerations for integration:
- Should
ForcePollbe able to trigger a poll even whenisTrackingis false?- How should
ForcePollinteract with the regular polling interval in thepollmethod?- Should there be a cooldown period between forced polls to prevent excessive API calls?
Example integration (to be adapted based on your specific requirements):
func (t *SFVTracker) ForcePoll() { if !t.isTracking { log.Println("Cannot force poll: tracking is not active") return } // Trigger an immediate poll go t.refreshMatchHistory(context.Background(), t.mh.CFN, false) // Optionally, reset the polling interval // This would require modifying the `poll` method to use a resettable timer }Remember to handle any potential race conditions and ensure thread safety when implementing this functionality.
gui/src/pages/tracking/tracking-live-updater.tsx (2)
120-142: LGTM: Good layout for new buttons, with a minor suggestion.The new button layout using flexbox is well-structured and provides a good user experience. The refresh button is properly implemented with disabled state handling.
For consistency, consider using the
tfunction for internationalization on the "Refresh" text, similar to how it's used for the "Stop" button:- Refresh + {t('refresh')}This will ensure that the "Refresh" text can be easily translated in the future if needed.
124-130: LGTM: Refresh functionality implemented well, with suggestions for improvement.The refresh functionality is implemented correctly, with a good approach to prevent rapid successive clicks. However, there are a few points to consider for improvement:
- The 15-second cooldown is hardcoded. Consider making this a configurable constant or prop.
- The use of
setTimeoutcould potentially lead to memory leaks if the component unmounts before the timeout completes. Consider usinguseEffectfor cleanup:React.useEffect(() => { let timeoutId: number; if (refreshDisabled) { timeoutId = window.setTimeout(() => setRefreshDisabled(false), 15000); } return () => { if (timeoutId) window.clearTimeout(timeoutId); }; }, [refreshDisabled]);
- Consider adding error handling for the
trackingActor.sendcall, in case the action fails.Would you like me to provide a more detailed implementation of these suggestions?
cmd/cmd.go (1)
225-229: LGTM! Consider adding error handling.The
ForcePollmethod is a good addition that aligns with the PR objective of forcing polling for t8. The nil check onch.trackeris a good practice to prevent nil pointer dereference.Consider adding error handling to propagate any errors that might occur during the poll:
-func (ch *CommandHandler) ForcePoll() { +func (ch *CommandHandler) ForcePoll() error { if ch.tracker != nil { - ch.tracker.ForcePoll() + return ch.tracker.ForcePoll() } + return nil }This change assumes that
ForcePoll()on the tracker returns an error. If it doesn't, you may need to modify the tracker interface to include error handling.pkg/tracker/sf6/track.go (1)
324-324: Consider the integration ofForcePollwith the existing polling mechanism.The addition of
ForcePollintroduces a new way to interact with the polling mechanism. To ensure proper integration and avoid potential conflicts, consider the following suggestions:
- Update the
SF6Trackerstruct to include a mutex for thread-safe access to shared resources:type SF6Tracker struct { // ... existing fields ... pollMutex sync.Mutex }
- Modify the
Start,Stop, andpollmethods to use this mutex:func (t *SF6Tracker) Start(ctx context.Context, userCode string, restore bool, pollRate time.Duration) error { t.pollMutex.Lock() defer t.pollMutex.Unlock() // ... existing implementation ... } func (t *SF6Tracker) Stop() { t.pollMutex.Lock() defer t.pollMutex.Unlock() t.stopPolling() } func (t *SF6Tracker) poll(ctx context.Context, userCode string, pollRate time.Duration) { t.pollMutex.Lock() defer t.pollMutex.Unlock() // ... existing implementation ... }
- Implement
ForcePollto work harmoniously with the existing polling mechanism:func (t *SF6Tracker) ForcePoll(ctx context.Context) error { t.pollMutex.Lock() defer t.pollMutex.Unlock() if !t.isAuthenticated { return errors.New("tracker not authenticated") } bl, err := t.fetchBattleLog(t.user.Code) if err != nil { return fmt.Errorf("failed to fetch battle log: %w", err) } return t.updateSession(ctx, bl) }
- Consider adding a state variable to track whether polling is active, and use it in
ForcePoll:type SF6Tracker struct { // ... existing fields ... pollMutex sync.Mutex isPolling bool } func (t *SF6Tracker) ForcePoll(ctx context.Context) error { t.pollMutex.Lock() defer t.pollMutex.Unlock() if !t.isAuthenticated { return errors.New("tracker not authenticated") } if !t.isPolling { return errors.New("polling is not active") } // ... rest of the implementation ... }These changes will help ensure that
ForcePollintegrates well with the existing polling mechanism and maintains thread-safety.Would you like assistance in implementing these changes or further refining the polling mechanism?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- cmd/cmd.go (1 hunks)
- gui/src/pages/tracking/tracking-live-updater.tsx (3 hunks)
- gui/src/state/tracking-machine.ts (3 hunks)
- pkg/tracker/sf6/track.go (1 hunks)
- pkg/tracker/sfv/track.go (1 hunks)
- pkg/tracker/t8/track.go (7 hunks)
- pkg/tracker/tracker.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
pkg/tracker/tracker.go (1)
12-12: Verify implementation ofForcePoll()across the codebaseThe addition of the
ForcePoll()method to theGameTrackerinterface requires updates to all its implementations. Let's verify this change has been properly propagated.Run the following script to check for
GameTrackerimplementations and theirForcePoll()methods:This script will help us ensure that:
- All
GameTrackerimplementations have been updated with theForcePoll()method.- The
ForcePoll()method is being used appropriately in the codebase.Please review the script output and make any necessary adjustments to ensure full implementation and proper usage of the new method.
gui/src/state/tracking-machine.ts (1)
4-4: LGTM: New import added correctly.The
ForcePollfunction is imported from the@cmdmodule, which is consistent with the existing import style and is used in the newforcePollaction.gui/src/pages/tracking/tracking-live-updater.tsx (2)
34-35: LGTM: New state variable for managing refresh button state.The addition of the
refreshDisabledstate variable is a good approach for managing the enabled/disabled state of the refresh button. The naming is clear and the initial value offalseis appropriate.
Line range hint
1-143: Overall, good implementation of the refresh functionality with room for minor improvements.The changes to the
TrackingLiveUpdatercomponent successfully implement the new refresh functionality. The code is well-structured and follows React best practices. Here's a summary of the main points:
- The new state variable
refreshDisabledis appropriately used to manage the refresh button's state.- The button layout is well-designed using flexbox.
- The refresh functionality is correctly implemented with a cooldown mechanism.
Consider the following improvements:
- Use internationalization for the "Refresh" button text.
- Make the cooldown duration configurable.
- Implement proper cleanup for the setTimeout using useEffect.
- Add error handling for the trackingActor.send call.
These changes will enhance the maintainability and robustness of the component.
cmd/cmd.go (2)
225-229: Summary: ForcePoll method addition enhances CommandHandler functionality.The addition of the
ForcePollmethod to theCommandHandlerstruct is a well-implemented feature that aligns with the PR objective of forcing polling for t8. It integrates seamlessly with the existing code structure and enhances the flexibility of the tracking system.Key points:
- The method correctly checks for a nil tracker before calling
ForcePoll().- It provides a way to manually trigger a poll action, complementing existing tracking functionality.
- The change doesn't introduce any breaking changes to the existing code structure.
Suggestions for improvement:
- Consider adding error handling to propagate any errors that might occur during the poll.
- Verify that the
GameTrackerinterface includes theForcePollmethod to ensure consistency across the codebase.Overall, this is a solid addition that enhances the
CommandHandler's capabilities.
225-229: Verify GameTracker interface includes ForcePoll method.The new
ForcePollmethod integrates well with the existingCommandHandlerfunctionality. It enhances the tracking system by allowing manual poll triggers without introducing breaking changes.Please run the following script to verify that the
GameTrackerinterface includes theForcePollmethod:If the script doesn't find a match, you may need to update the
GameTrackerinterface to include theForcePollmethod.pkg/tracker/t8/track.go (1)
114-117:⚠️ Potential issueSeparate OS signal handling from force polling
Using
t.forcePollChanto handle both OS interrupts and force polling can cause confusion and potential conflicts. Receiving an OS interrupt might unintentionally trigger a forced poll.Modify the select case to work with the updated channel type:
for { select { - case <-t.forcePollChan: + case <-t.forcePollChan: i++ log.Println("forced poll", i) t.pollFn(ctx, session)Ensure that OS signals are handled separately if needed.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
gui/src/ui/tooltip.tsx (1)
8-21: LGTM: Well-structured component with good use of conditional rendering and animations.The component structure is clean and makes good use of conditional rendering for the tooltip. The use of Framer Motion for animations is a nice touch for smooth transitions.
Consider adding an
aria-labelortitleattribute to the wrapper div for improved accessibility, especially when the tooltip is disabled.<div className="group z-[9999] relative inline-flex justify-center" + aria-label={props.disabled ? undefined : props.text} >pkg/i18n/locales/en_GB.go (1)
65-66: LGTM! Consider adding comments for context.The new entries for "Refresh" and "Cooldown" are correctly added and appropriately translated for British English. These additions align well with the PR's objective of implementing force polling.
For consistency and to aid future maintenance, consider adding comments to explain the context or usage of these new entries, similar to how some other entries in this file might have comments (if applicable). For example:
// Used for the refresh button in the polling interface Refresh: "Refresh", // Indicates the cooldown period between polls Cooldown: "Cooldown",gui/src/pages/tracking/tracking-live-updater.tsx (2)
56-56: LGTM: Improved layout structure.The new flex container improves the overall layout and allows for better positioning of the new refresh button.
Consider adding a comment explaining the calculation
h-[calc(100%-32px)]for better maintainability.
121-137: LGTM: Well-implemented refresh button with cooldown logic.The refresh button is correctly implemented with proper state management, cooldown logic, and integration with the
trackingActor. The use ofTooltipfor cooldown indication enhances the user experience.Consider extracting the cooldown duration (15000ms) into a constant or configuration value for easier maintenance and potential future customization.
+ const REFRESH_COOLDOWN_MS = 15000; // ... - setTimeout(() => setRefreshDisabled(false), 15000) + setTimeout(() => setRefreshDisabled(false), REFRESH_COOLDOWN_MS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- gui/src/pages/matches.tsx (2 hunks)
- gui/src/pages/tracking/tracking-live-updater.tsx (6 hunks)
- gui/src/ui/tooltip.tsx (1 hunks)
- pkg/i18n/locales/en_GB.go (1 hunks)
- pkg/i18n/locales/fr_FR.go (1 hunks)
- pkg/i18n/locales/ja_JP.go (1 hunks)
- pkg/i18n/locales/model.go (1 hunks)
- pkg/tracker/t8/track.go (5 hunks)
🧰 Additional context used
🔇 Additional comments (12)
gui/src/ui/tooltip.tsx (2)
1-2: LGTM: Imports are correct and necessary.The imports for React and Framer Motion's motion component are appropriate for this Tooltip component.
4-7: LGTM: Well-structured component declaration with proper typing.The Tooltip component is correctly exported and uses appropriate TypeScript typing for its props. The use of React.PropsWithChildren is a good practice for components that wrap other elements.
pkg/i18n/locales/ja_JP.go (1)
65-66: LGTM! Translations are correct and consistent.The added translations for "Refresh" (リフレッシュ) and "Cooldown" (クールダウン) are correct and commonly used in Japanese. They are consistent with the style of other entries in the file, using appropriate katakana for foreign-derived words. These additions align well with the new polling feature mentioned in the PR title.
pkg/i18n/locales/fr_FR.go (1)
65-66: Approve "Refresh" translation and suggest improvement for "Cooldown"The translation for "Refresh" as "Rafraîchir" is correct and appropriate. However, the translation for "Cooldown" as "Refroidir" might not be the most suitable in this context.
For "Cooldown", consider using "Temps de recharge" or "Récupération" instead of "Refroidir". These terms are more commonly used in French gaming contexts to represent a cooldown period.
Suggested change:
Refresh: "Rafraîchir", -Cooldown: "Refroidir", +Cooldown: "Temps de recharge",To ensure consistency across other language files, you can run the following command:
This will help verify if other language files have a translation for "Cooldown" and how it's translated.
✅ Verification successful
Update "Cooldown" translation for better accuracy
The current translation for "Cooldown" as "Refroidir" may not accurately reflect its intended meaning in this context. Consider using "Temps de recharge" instead.
Suggested change:
Refresh: "Rafraîchir", -Cooldown: "Refroidir", +Cooldown: "Temps de recharge",🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the presence and translation of "Cooldown" in other language files rg --type go -i 'cooldown.*:' pkg/i18n/localesLength of output: 319
gui/src/pages/matches.tsx (3)
12-12: LGTM: New import for Tooltip componentThe new import for the
Tooltipcomponent is correctly placed and follows good practices for importing internal components.
122-124: Great improvement: Simplified tooltip implementationThe changes to the replay ID cell rendering are a significant improvement:
- The code is more concise and readable by using the dedicated
Tooltipcomponent.- Passing the tooltip text as a prop enhances maintainability.
- The span for the replay ID correctly handles overflow with appropriate CSS classes.
- The functionality for copying the replay ID is preserved.
These changes align well with React best practices and improve the overall code quality.
Line range hint
1-134: Overall assessment: Excellent improvementsThe changes in this file are well-implemented and improve both the code quality and user experience:
- The introduction of the
Tooltipcomponent enhances the UI functionality.- The refactoring of the replay ID cell rendering simplifies the code and improves maintainability.
- These changes are seamlessly integrated without disrupting the existing functionality.
Great job on these improvements!
pkg/i18n/locales/model.go (1)
65-66: LGTM! Consider verifying related files.The addition of
RefreshandCooldownfields to theLocalizationstruct looks good. The naming and JSON tags are consistent with the existing convention.To ensure completeness, please verify that corresponding translations have been added to all language-specific localization files. Run the following script to check for the new keys in all JSON files:
This will help ensure that all necessary translations have been added for the new fields.
gui/src/pages/tracking/tracking-live-updater.tsx (4)
10-10: LGTM: New import for Tooltip component.The import for the
Tooltipcomponent is correctly placed and will be used for the new refresh button functionality.
35-36: LGTM: New state variable for refresh button.The
refreshDisabledstate variable is correctly implemented using theuseStatehook. Its purpose is clear, and it will be used to manage the disabled state of the new refresh button.
138-143: LGTM: Stop button properly integrated into new layout.The stop button has been correctly repositioned within the new flex container, maintaining its original functionality while aligning with the updated layout.
Line range hint
1-180: Overall: Well-implemented refresh functionality with minor suggestions for improvement.The changes to the
TrackingLiveUpdatercomponent effectively implement the new refresh functionality while maintaining the existing structure and functionality. The code is well-organized and follows React best practices. Consider implementing the suggested improvements for even better maintainability and flexibility.
williamsjokvist
force-pushed
the
feat/force-polling
branch
from
39279ce to
34f2aec
Compare
williamsjokvist
force-pushed
the
feat/force-polling
branch
from
34f2aec to
a249b59
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Documentation