Refactor WebApp for improved readability and structure #8
Conversation
Replaced try/catch with a safer and cleaner conditional check using optional chaining. Using try/catch for value assignment is not considered good practice, especially when the value can be checked with condition.
Replaced nested conditionals with simpler, more readable else-if structure. This reduces indentation levels and improves clarity when checking for exam duration and start time.
Reorganized the logic to reduce nesting. The counter is now checked first, and the exam starts when it reaches zero. The button text is updated only when necessary, improving readability and flow.
Removed explicit check for 'EVALUATING' status. Now we only throw on 'EVALUATION_FAILED' or other unexpected statuses. All other cases fall through to polling, making the logic cleaner and easier to follow.
Moved the success timeout logic inside the success condition block to improve readability and keep related logic together.
WalkthroughThe changes refactor several JavaScript modules within the application. In the UI utilities component, redundant success handling code was removed and streamlined. Cosmetic formatting adjustments were applied across multiple modules, including the addition of spaces before parentheses in function declarations and event listener callbacks. The index module simplified error handling for retrieving the user agent by replacing a try-catch with a conditional check. Overall, the functionality remains unchanged while improving code readability and consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResultsHandler
Client->>ResultsHandler: Request exam results
alt Status is EVALUATION_FAILED
ResultsHandler-->>Client: Throw evaluation failed error
else Status is not EVALUATED
ResultsHandler-->>Client: Throw "results not available" error
else Status is EVALUATED but results missing
Note right of ResultsHandler: Initiate polling for results
ResultsHandler->>Client: Begin polling process
end
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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 (
|
app/public/js/index.js
Outdated
| } | ||
|
|
||
| selectedLab.userAgent = userAgent; |
There was a problem hiding this comment.
Can we assignselectedLab.userAgent=navigator.userAgent; inside a if statement since userAgent not used anywhere else
There was a problem hiding this comment.
Absolutely.
I'll try this, evening if I have some time, to squeeze in this task
app/public/js/results.js
Outdated
| @@ -344,15 +344,13 @@ document.addEventListener('DOMContentLoaded', function() { | |||
| return response.json(); | |||
| }) | |||
| .then(data => { | |||
| if (data.status === 'EVALUATING') { | |||
| // Start polling if exam is being evaluated | |||
| startPolling(examId); | |||
There was a problem hiding this comment.
I think this is needed for for refresh scenario in the result page. removing this causes any side effects ?
There was a problem hiding this comment.
I don't think so because the other cases don't match this one and the last else does exactly the same if I'm right.
There was a problem hiding this comment.
Oh shut. You're actually right.
My bad the other scenario is with EVALUATED and not EVALUATING.
I'll rollback this one.
This reverts commit b65ad56. The reverted commit introduced a bug where the state 'EVALUATING' was not handled correctly. Thisone fixes it rolling back the logit as it was befor that change. How can we call that? A reverse typo? Let's just say I brainfarted: I thought that the second `else if` was against 'EVALUATING'. Too many hours coding, I'd say. Thanks @nishanb for the catch! Updated accordingly.
![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 (2)
app/public/js/panel-resizer.js (2)
57-60: Optional Chaining Suggestion for ClientX Retrieval:
On line 58, consider using optional chaining to simplify the access ofe.touches. This not only shortens the code but also improves readability.Apply the following diff:
- this.startX = e.clientX || (e.touches && e.touches[0].clientX) || 0; + this.startX = e.clientX || e.touches?.[0]?.clientX || 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
75-83: Optional Chaining Suggestion in thedragMethod:
Similarly, on line 79, using optional chaining for accessinge.touchescan modernize the code and reduce the verbosity of manual checks.Apply this diff:
- const clientX = e.clientX || (e.touches && e.touches[0].clientX) || 0; + const clientX = e.clientX || e.touches?.[0]?.clientX || 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/public/js/answers.js(6 hunks)app/public/js/app.js(1 hunks)app/public/js/feedback.js(2 hunks)app/public/js/index.js(20 hunks)app/public/js/panel-resizer.js(2 hunks)app/public/js/results.js(9 hunks)
✅ Files skipped from review due to trivial changes (3)
- app/public/js/app.js
- app/public/js/answers.js
- app/public/js/feedback.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/public/js/results.js
- app/public/js/index.js
🧰 Additional context used
🪛 Biome (1.9.4)
app/public/js/panel-resizer.js
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 79-79: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
app/public/js/panel-resizer.js (6)
10-25: Cosmetic Formatting in Constructor:
The removal of unnecessary blank lines and the consistent spacing in the constructor (affecting lines 12, 16, 19, 22, and 25) improves readability without altering functionality.
26-35: Cosmetic Formatting in theinitMethod:
Adjustments made on lines 28 and 35 (and similar spacing changes) help clarify the code structure while keeping the logic intact.
50-73: Cosmetic Formatting in thestartDragMethod:
Minor spacing adjustments (e.g., on lines 53 and 56) enhance clarity in this method, ensuring a cleaner visual flow during drag initialization.🧰 Tools
🪛 Biome (1.9.4)
[error] 58-58: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
112-121: Cosmetic Formatting in thestopDragMethod:
The removal or adjustment of extraneous blank lines (e.g., on lines 114, 118, and 121) improves code consistency and readability in this method.
127-131: Cosmetic Formatting in theresetPanelsMethod:
A minor formatting change on line 131 helps maintain a consistent code style.
139-151: Event Listener Formatting for DOMContentLoaded:
The addition of a space before the opening parenthesis in the callback registration forDOMContentLoaded(line 139 and onward) standardizes the code formatting.
|
@davidescapolan01 thanks for these changes. I believe you have refactored the code and tested E2E. since we do not have any unit test or E2E test making sure it works before merging the changes !! |
Hm, actually I didn't — my bad! Would it be okay if I mark this PR as a Draft and come back to it once I’ve managed to test it end-to-end? Also, what kind of tests should I perform? I'm asking because I’m not sure I have the skills yet to write proper frontend tests. |
What this PR does
This PR refactors several parts of the code related to connection status handling, condition simplification, and event logic inside the WebApp.
It improves readability by reducing nesting, grouping related logic, and replacing unnecessary try/catch blocks with proper conditional checks.
Why it's needed
Some sections of the code used unnecessary try/catch blocks or scattered related logic across conditionals.
These changes streamline the logic, make the behavior easier to follow, and reduce potential confusion for future maintenance.
How to test
Expected result
Comment for the Contributor
Hey! 👋 Just wanted to streamline a few things in the logic—especially around how conditions are written and grouped.
This should make the flow more readable and a bit easier to maintain.
Let me know if you want me to split anything or adjust further!
Summary by CodeRabbit
Refactor
Style