SSR default classifier pages with loader fallback#12
Open
DmitryMatv wants to merge 3 commits intomainfrom
Open
SSR default classifier pages with loader fallback#12DmitryMatv wants to merge 3 commits intomainfrom
DmitryMatv wants to merge 3 commits intomainfrom
Conversation
- Render base classifier pages with server-side example results - Keep the client loader for search pages or SSR failures - Add coverage for SSR success and fallback behavior
📝 WalkthroughWalkthroughAdds a centralized helper to build classification results context, refactors server-side rendering flow to optionally SSR results with a client-side fallback, tightens the template conditional so the HTMX initial-results-loader mounts only when both Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server (app/web)
participant Classifier as Classifier Service
participant Template as Template Renderer
Client->>Server: GET /<classifier>[/query]
alt URL contains search query or example-query SSR path
Server->>Classifier: perform_classification(query, version, top_k)
Classifier-->>Server: results
Server->>Server: build_classification_results_context(...)
Server->>Template: render page with inlined results (needs_initial_results_loader=false)
Template-->>Client: HTML (results inlined)
else SSR disabled or classification raises error
Server->>Server: set trigger_search_on_load=true, needs_initial_results_loader=true
Server->>Template: render page with `initial-results-loader` (HX loader)
Template-->>Client: HTML (client will fetch results)
Client->>Server: HX request for classification fragment (on load)
Server->>Classifier: perform_classification(...)
Classifier-->>Server: results fragment
Server-->>Client: results fragment (replace loader)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/web.py (1)
576-592: The broadExceptioncatch is acceptable for SSR fallback, but consider logging at warning level.The static analysis flags catching bare
Exception, but this is a valid pattern for optional SSR with graceful degradation. The current implementation correctly:
- Logs the error
- Falls back to client-side loading
However, since
perform_classificationconverts most errors toHTTPException, you're also catching HTTP 5xx errors (e.g., backend unavailable). For operational visibility, consider usinglogger.warninginstead oflogger.errorto distinguish "expected fallback" from "unexpected failure," or log the exception type:♻️ Optional: Add exception type for debugging
except Exception as e: logger.error( - "Error during '%s' page SSR classification: %s", + "Error during '%s' page SSR classification (%s): %s", effective_classifier_type, + type(e).__name__, e, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web.py` around lines 576 - 592, Change the error-level logging to a warning and include the exception type/message when the SSR classification fallback occurs: inside the except block that surrounds build_classification_results_context (referencing build_classification_results_context, effective_classifier_type, trigger_search_on_load, and needs_initial_results_loader), replace the logger.error call with logger.warning and format the log to include both the exception type and the exception instance so the fallback remains quiet but still records useful operational context.tests/test_web_helpers.py (1)
326-344: Consider making the example query assertion more robust.Line 340 asserts
"Apple MacBook Air (13-inch, M3, 2024)"appears in the response, which depends on the current value ofCLASSIFIER_CONFIG["UNSPSC"]["example"]. If this config value changes, the test will fail for reasons unrelated to the SSR fallback behavior being tested.Consider deriving the expected text from the config:
expected_example = CLASSIFIER_CONFIG["UNSPSC"]["example"].replace("Example:", "").strip() self.assertIn(expected_example, response.text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_web_helpers.py` around lines 326 - 344, The hard-coded example string in test_base_page_ssr_failure_falls_back_to_loader makes the test brittle; instead derive the expected text from CLASSIFIER_CONFIG to match whatever the config currently contains. Replace the literal assertion that checks for "Apple MacBook Air (13-inch, M3, 2024)" with code that reads CLASSIFIER_CONFIG["UNSPSC"]["example"], strips any leading "Example:" and whitespace (e.g., .replace("Example:", "").strip()), then assert that this expected_example is in response.text; keep the other assertions and mocks (perform_classification_mock, check_usage_mock, increment_usage_mock) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/web.py`:
- Line 584: Remove the unused variable has_server_rendered_results: locate and
delete its assignments (the occurrences that set has_server_rendered_results =
True) and any standalone declaration; ensure no other code (template context
construction or conditionals) references has_server_rendered_results and, if it
does, either remove that reference or replace it with the appropriate used
variable. Confirm imports or lints are clean after removal.
---
Nitpick comments:
In `@app/web.py`:
- Around line 576-592: Change the error-level logging to a warning and include
the exception type/message when the SSR classification fallback occurs: inside
the except block that surrounds build_classification_results_context
(referencing build_classification_results_context, effective_classifier_type,
trigger_search_on_load, and needs_initial_results_loader), replace the
logger.error call with logger.warning and format the log to include both the
exception type and the exception instance so the fallback remains quiet but
still records useful operational context.
In `@tests/test_web_helpers.py`:
- Around line 326-344: The hard-coded example string in
test_base_page_ssr_failure_falls_back_to_loader makes the test brittle; instead
derive the expected text from CLASSIFIER_CONFIG to match whatever the config
currently contains. Replace the literal assertion that checks for "Apple MacBook
Air (13-inch, M3, 2024)" with code that reads
CLASSIFIER_CONFIG["UNSPSC"]["example"], strips any leading "Example:" and
whitespace (e.g., .replace("Example:", "").strip()), then assert that this
expected_example is in response.text; keep the other assertions and mocks
(perform_classification_mock, check_usage_mock, increment_usage_mock) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cccd0db-9ac1-4b7c-b0ba-264f65f03a4c
📒 Files selected for processing (4)
app/templates/classifier_page.htmlapp/web.pytests/test_request_validation_and_metadata.pytests/test_web_helpers.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5b84cb552
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Extend Cloudflare cache TTL for HTML pages - Log SSR classification failures as warnings and fall back to client load - Update test to assert the configured example is rendered
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/web.py (1)
584-589: Include traceback in SSR fallback warning logs.Fallback is correct, but logging only the exception message makes production debugging harder. Keep fallback behavior and include stack traces.
Suggested patch
- logger.warning( - "SSR fallback for '%s' page classification due to %s: %s", - effective_classifier_type, - type(e).__name__, - e, - ) + logger.warning( + "SSR fallback for '%s' page classification due to %s: %s", + effective_classifier_type, + type(e).__name__, + e, + exc_info=True, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/web.py` around lines 584 - 589, The SSR fallback warning currently logs only the exception type and message in the logger.warning call around SSR fallback for effective_classifier_type; change the log call to include the traceback (e.g., add exc_info=True to the existing logger.warning invocation or use logger.exception if you want an ERROR-level stack trace) so the same message is emitted but with full stack trace for debugging; keep the fallback behavior unchanged and only modify the logger.warning(...) line that references effective_classifier_type, type(e).__name__, and e to include exc_info=True.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/web.py`:
- Around line 584-589: The SSR fallback warning currently logs only the
exception type and message in the logger.warning call around SSR fallback for
effective_classifier_type; change the log call to include the traceback (e.g.,
add exc_info=True to the existing logger.warning invocation or use
logger.exception if you want an ERROR-level stack trace) so the same message is
emitted but with full stack trace for debugging; keep the fallback behavior
unchanged and only modify the logger.warning(...) line that references
effective_classifier_type, type(e).__name__, and e to include exc_info=True.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a7d45b9-4d97-44a1-aca8-c741f11989f9
📒 Files selected for processing (3)
app/cache_profiles.pyapp/web.pytests/test_web_helpers.py
✅ Files skipped from review due to trivial changes (1)
- app/cache_profiles.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Not runin this environment.tests/test_web_helpers.pyfor SSR success and fallback cases.tests/test_request_validation_and_metadata.pyfor base-page loader fallback.Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Chores