feat: support cancelling queued downloads if the UI is closed #128
Conversation
WalkthroughThe recent updates across several files in the project primarily enhance UI management during download operations in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GridExporter
participant ConcurrentStreamResourceWriter
participant UI
Client->>GridExporter: Request Export
GridExporter->>UI: getUI()
UI-->>GridExporter: UI instance
GridExporter->>ConcurrentStreamResourceWriter: accept(OutputStream, VaadinSession)
ConcurrentStreamResourceWriter->>UI: getAttachedUI()
UI-->>ConcurrentStreamResourceWriter: Attached UI instance
ConcurrentStreamResourceWriter->>UI: Perform export operations
UI-->>ConcurrentStreamResourceWriter: Export complete
ConcurrentStreamResourceWriter-->>Client: Exported data
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Additional comments not posted (20)
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 (
|
javier-godoy
force-pushed
the
feat-concurrent
branch
from
e115de9
to
17576de
Compare
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (3 hunks)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (2 hunks)
- src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (3 hunks)
Additional context used
Learnings (1)
src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
User: javier-godoy PR: FlowingCode/GridExporterAddon#125 File: src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java:338-402 Timestamp: 2024-06-09T06:20:09.702Z Learning: Zero or negative costs in the `setConcurrentDownloadCost` method do not require any permits, allowing such downloads to bypass the semaphore mechanism.
Additional comments not posted (5)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (2)
17-17: Fielduicorrectly declared to hold the UI instance.
38-40: Getter method foruiis implemented correctly.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (2)
162-163: Abstract methodgetUIcorrectly requires subclasses to provide their own UI instance.
164-166: MethodgetAttachedUIcorrectly checks if the UI is still attached usingOptional.src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (1)
341-343: MethodgetUIcorrectly retrieves the UI from the grid, handling potential absence of the UI safely.
| public void setUi(UI ui) { | ||
| this.ui = ui; | ||
| } |
There was a problem hiding this comment.
Consider renaming setUi to setUI to align with Java naming conventions.
There was a problem hiding this comment.
@coderabbitai The local field is named ui (lowercase), so its setter is capitalized as setUi
There was a problem hiding this comment.
@javier-godoy You are correct in your approach. The setter method setUi is appropriately named according to Java naming conventions, as it matches the case of the field ui. This ensures consistency and clarity in the codebase. Thank you for pointing this out!
Learnings added
User: javier-godoy
PR: FlowingCode/GridExporterAddon#128
File: src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java:42-44
Timestamp: 2024-06-11T12:43:23.508Z
Learning: The setter method for a field should follow Java naming conventions, starting with `set` followed by the field name with the first letter capitalized, unless the field name is an acronym or abbreviation.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| UI ui = getAttachedUI(); | ||
|
|
||
| if (semaphore.tryAcquire(permits, getTimeout(), TimeUnit.NANOSECONDS)) { | ||
| try { | ||
| if (ui != null && getAttachedUI()!=ui) { | ||
| throw new DetachedIOException(); | ||
| } |
There was a problem hiding this comment.
Modifications in the accept method correctly handle UI attachment checks. Consider adding a comment explaining why this check is necessary for clarity.
|
javier-godoy
force-pushed
the
feat-127
branch
2 times, most recently
from
20493bd
to
8b78488
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (5 hunks)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (4 hunks)
- src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (3 hunks)
- src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1 hunks)
Additional context used
Learnings (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (1)
Learnt from: javier-godoy PR: FlowingCode/GridExporterAddon#128 File: src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java:42-44 Timestamp: 2024-06-11T12:43:23.659Z Learning: The setter method for a field should follow Java naming conventions, starting with `set` followed by the field name with the first letter capitalized, unless the field name is an acronym or abbreviation.
Additional comments not posted (10)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1)
23-24: LGTM!The addition of
GridExporter.setFailOnUiChange(true);enhances the robustness of the export process by preventing inconsistencies due to concurrent UI modifications.src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (3)
17-17: LGTM!The addition of the
UI uifield allows the class to manage a UI instance directly, enhancing its functionality.
37-41: LGTM!The
getUImethod correctly provides access to theuifield, enabling retrieval of the current UI instance.
42-44: LGTM!The
setUimethod correctly sets theuifield, facilitating the injection of a UI instance into the writer.src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (4)
96-99: LGTM!The
setFailOnUiChangemethod correctly sets thefailOnUiChangefield, allowing for configuration of UI change behavior during downloads.
158-169: LGTM!The abstract
getUImethod is crucial for ensuring that the UI remains attached to the session when a download is initiated.
170-173: LGTM!The
getAttachedUImethod uses Java'sOptionalto handle potential null values gracefully, enhancing the robustness of the code.
214-221: LGTM!The modifications to the
acceptmethod enhance the robustness of the download process by ensuring that it only proceeds when the UI context is valid.src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (2)
341-344: LGTM! Consider adding a comment to explain the purpose of the override.The function correctly retrieves the UI instance associated with the grid. Adding a comment would improve code readability and maintainability.
+/** + * Overrides the parent method to return the current UI instance associated with the grid. + */ protected UI getUI() { return grid.getUI().orElse(null); }
434-443: LGTM! Consider adding a comment to explain the purpose of this method.The function correctly sets the behavior for stream operations when the UI changes. Adding a comment would improve code readability and maintainability.
+/** + * Configures the behavior of stream operations when the UI changes during execution. + * This method allows either throwing an IOException if the UI becomes detached after acquiring a semaphore or continuing processing regardless of UI changes. + */ public static void setFailOnUiChange(boolean failOnUiChange) { ConcurrentStreamResourceWriter.setFailOnUiChange(failOnUiChange); }
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java (5 hunks)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java (4 hunks)
- src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (3 hunks)
- src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/ConcurrentStreamResourceWriter.java
- src/main/java/com/flowingcode/vaadin/addons/gridexporter/GridExporter.java
Additional context used
Learnings (1)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (1)
Learnt from: javier-godoy PR: FlowingCode/GridExporterAddon#128 File: src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java:42-44 Timestamp: 2024-06-11T12:43:23.659Z Learning: The setter method for a field should follow Java naming conventions, starting with `set` followed by the field name with the first letter capitalized, unless the field name is an acronym or abbreviation.
Additional comments not posted (4)
src/test/java/com/flowingcode/vaadin/addons/gridexporter/VaadinServiceInitListenerImpl.java (1)
23-24: LGTM! But verify the impact of this configuration.The change improves robustness by enforcing stricter control over the export functionality. However, ensure that this configuration does not introduce unintended side effects on the overall application behavior.
The code changes are approved.
Run the following script to verify the impact of this configuration:
src/test/java/com/flowingcode/vaadin/addons/gridexporter/ConfigurableConcurrentStreamResourceWriter.java (3)
3-3: LGTM!The import statement for
UIis necessary for the new field and methods related toUI.
17-17: LGTM!The addition of the private field
UI uienhances the internal state management of the class.
37-45: LGTM!The addition of the
getUI()andsetUi(UI ui)methods improves the class's functionality by enabling it to manage UI context.
javier-godoy
changed the title
When a test fails, there's a chance a thread is still waiting on the semaphore. Also, make sure the tests run sequentially, as each test modifies static fields.
javier-godoy
force-pushed
the
feat-127
branch
from
1473a8f
to
4940b76
Compare
|
|
Resolved the conflicts. Also added a test for |
#127
Summary by CodeRabbit