[Web] Fix report session truncation#304
Conversation
git-miku
Bot
added
the
needs-triage
git-miku
Bot
added
size/S
|
Tip 👋 Hey @floze-the-genius — Miku's on it. Here are the most useful commands for this PR:
Note 🎓 This repo is part of ECSOC26. Run Note ⭐ Star this repo to unlock all commands. Say 📖 All commands🤖 AI-powered — 14 commands
🔧 Issue & PR management — 18 commands
🎉 Community & utility — 9 commands
|
|
| Filename | Overview |
|---|---|
| termstory/web.py | Introduces WEB_REPORT_SESSION_LIMIT / AI_HIGHLIGHT_LIMIT constants and a session_window metadata block; the truncated flag and total count are incorrect when date filters are active because they compare against the unfiltered DB total rather than the filtered session count. |
| tests/test_web.py | Adds session_window assertions and imports WEB_REPORT_SESSION_LIMIT; all tests exercise the unfiltered path or a filtered path where all sessions fall within the filter range, so the date-filtered truncation bug goes untested. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller
participant get_web_data
participant analyze_all
participant DB
Caller->>get_web_data: get_web_data(db, start_ts, end_ts)
get_web_data->>analyze_all: analyze_all(db)
analyze_all->>DB: "COUNT(*) — no date filter"
DB-->>analyze_all: total_sessions (unfiltered)
analyze_all-->>get_web_data: "stats{total_sessions}"
get_web_data->>DB: SELECT id FROM sessions [WHERE …] LIMIT WEB_REPORT_SESSION_LIMIT
DB-->>get_web_data: session_ids (filtered + limited)
Note over get_web_data: session_window.total = stats[total_sessions] (unfiltered!)
Note over get_web_data: truncated = total_sessions > len(sessions_data) ⚠ wrong under date filter
get_web_data-->>Caller: "{stats, sessions, session_window, …}"
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller
participant get_web_data
participant analyze_all
participant DB
Caller->>get_web_data: get_web_data(db, start_ts, end_ts)
get_web_data->>analyze_all: analyze_all(db)
analyze_all->>DB: "COUNT(*) — no date filter"
DB-->>analyze_all: total_sessions (unfiltered)
analyze_all-->>get_web_data: "stats{total_sessions}"
get_web_data->>DB: SELECT id FROM sessions [WHERE …] LIMIT WEB_REPORT_SESSION_LIMIT
DB-->>get_web_data: session_ids (filtered + limited)
Note over get_web_data: session_window.total = stats[total_sessions] (unfiltered!)
Note over get_web_data: truncated = total_sessions > len(sessions_data) ⚠ wrong under date filter
get_web_data-->>Caller: "{stats, sessions, session_window, …}"
Reviews (4): Last reviewed commit: "fix(web): surface truncated session repo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the Greptile P2 in the latest commit: the regression tests now import |
Makes sense. I’ll triage it by the end of the day. Thanks for the contribution. |
![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: bd18a5e01b
ℹ️ 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".
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| WEB_REPORT_SESSION_LIMIT = 1000 |
There was a problem hiding this comment.
When an unfiltered database has at least 1000 sessions, this limit now flows into db.get_sessions_by_ids(session_ids), and that path builds WHERE id IN (...) queries over all returned IDs (including _build_sessions' batched command fetch). On SQLite builds that still use the common 999 host-parameter cap, the new unfiltered report path raises sqlite3.OperationalError: too many SQL variables instead of generating the report; chunking the fetches or keeping the window below the cap would avoid that runtime failure.
Useful? React with 👍 / 👎.
|
@codex did i trigger you ? |
|
To use Codex here, create an environment for this repo. |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approve with minor suggestions — correct fix for silent 30-session unfiltered truncation; good constants + metadata + tests.
Intent
Unfiltered web reports were capped at 30 sessions while filtered used 1000. Unify on a named WEB_REPORT_SESSION_LIMIT (1000), parameterize LIMIT, and expose session_window metadata (issue #300).
Impact
- Unfiltered path no longer special-cased to 30 — real bugfix.
AI_HIGHLIGHT_LIMITnamed constant — clarity win.- Tests: empty window metadata, 31-session unfiltered no-truncation, and limit+5 truncated path updated to use the constant.
What’s correct
- Single
ORDER BY ... LIMIT ?path for both filtered and unfiltered. truncateduses stats total vs returned length; when a date range is applied, stats are overridden to the filtered counts first — so metadata stays coherent.- Parameter binding for LIMIT avoids injection and is SQLite-safe (single bind, not a 1000-id IN list).
Suggestions
- Template/UI consumption —
session_windowis returned in JSON but (if the HTML template doesn’t read it yet) users still won’t see truncation. Either wire a small banner in the report UI or note a follow-up issue so metadata isn’t dead API. - Optional: also return
filtered_totalviaCOUNT(*)with the same WHERE (without LIMIT) sototaldoesn’t depend on the stats override path — more direct, same result. - Performance note only: loading 1000 full sessions (commands/commits) can be heavy on large DBs; acceptable for the stated product choice, but worth watching.
Verdict
Comment / soft approve — core fix is right and well tested; template surfacing is the main follow-up.
| "returned": len(sessions_data), | ||
| "total": total_sessions, | ||
| "truncated": total_sessions > len(sessions_data), | ||
| } |
There was a problem hiding this comment.
Good metadata shape. Confirm the embedded report template actually reads session_window (banner like “Showing 1000 of 4500 sessions”). If not, either wire it in this PR or open a follow-up so this field isn’t unused JSON.
There was a problem hiding this comment.
Addressed in df859cc. The embedded report now renders an aria-live status banner when session_window.truncated is true (for example, “Showing 1000 of 4500 sessions”), and tests/test_web.py asserts both the banner wiring and truncated metadata. Focused web suite passes locally.
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| WEB_REPORT_SESSION_LIMIT = 1000 |
There was a problem hiding this comment.
Nice named constants. Using LIMIT ? with a single bound int is correct — no SQLite bind-variable explosion here (that concern applies to huge IN (...) lists, not this).
|
/triage |
git-miku
Bot
removed
the
needs-triage
|
miku ✅ Marked as triaged ( |
|
:miku /check-star |
git-miku
Bot
added
the
do-not-merge/needs-star
|
miku @floze-the-genius still hasn't starred this repo. Applied Star this repo first, then run |
|
:miku /check-star |
git-miku
Bot
added
needs-triage
|
Tip 👋 Hey @floze-the-genius — Miku's on it. Here are the most useful commands for this PR:
Note 🎓 This repo is part of ECSOC26. Run Note ⭐ Star this repo to unlock all commands. Say 📖 All commands🤖 AI-powered — 14 commands
🔧 Issue & PR management — 18 commands
🎉 Community & utility — 9 commands
|
|
miku ✅ @floze-the-genius has starred the repo — removed |
git-miku
Bot
added
size/M
|
Implemented the template/UI suggestion in |
df859cc to
90dea0b
Compare
|
Rebased onto |
| total_sessions = stats["total_sessions"] | ||
| session_window = { | ||
| "limit": WEB_REPORT_SESSION_LIMIT, | ||
| "returned": len(sessions_data), | ||
| "total": total_sessions, | ||
| "truncated": total_sessions > len(sessions_data), | ||
| } |
There was a problem hiding this comment.
truncated and total are wrong when a date filter is active
stats["total_sessions"] comes from analyze_all(db) on line 23, which counts every session in the database with no date filter. When start_ts/end_ts are supplied and the matching window holds fewer than WEB_REPORT_SESSION_LIMIT sessions, total_sessions will be larger than len(sessions_data) even though no truncation occurred — so truncated is True and total shows the full-DB count instead of the in-range count. Concretely: 600 sessions in DB, 50 match the date filter → UI shows "Showing 50 of 600 sessions" even though all 50 were returned. A separate COUNT(*) query that applies the same WHERE conditions as the session fetch is needed to populate total accurately before computing truncated.
|
/review |
|
/rewrite |
|
miku This looks solid. The unification of the session limit logic is clean—removing the redundant if/else branch that both applied The Test coverage is good—the new regression test for 31 sessions confirms the fix for the original issue, and updating Ready to merge. |
|
miku ✅ PR description rewritten! Check the PR body above. PreviewSummaryUnifies session limit logic for web reports under named constants, adds transparent truncation metadata, and displays a visible notice when reports are truncated. ChangesCore
UI
Tests
Architecture / Flowflowchart LR
A[get_web_data] --> B[Query Sessions]
B --> C[Apply WEB_REPORT_SESSION_LIMIT]
C --> D[Build session_window Metadata]
D --> E[Return to Frontend]
E --> F{truncated?}
F -->|Yes| G[Show Notice]
F -->|No| H[Hide Notice]
Fixes
Testing
Notes
|
|
/lgtm /approve |
![git-miku[bot]](https://avatars.githubusercontent.com/in/4140752?s=60&v=4){kind=small}
|
miku ✅ LGTM! Label added by @bitflicker64. Use miku ✅ Approved! 🎉 |
|
/ecsoc |
|
miku 🏷️ Added |
|
/applaud @floze-the-genius |
|
miku 🏆 @floze-the-genius — you absolutely killed it. 🎉
— Nominated by @bitflicker64 🙌✨ |
|
/label good-pr |
|
miku 🏷️ Added label |
|
/label good-backend |
|
miku 🏷️ Added label |
|
Please ask the ECSoC admin to check why the bot didn’t apply the ECSoC label. If you don’t have any connections, I’ll look into it myself. Just say the word or mention me here. |
2 participants
Summary
Unifies session limit logic for web reports under named constants, adds transparent truncation metadata, and displays a visible notice when reports are truncated.
Changes
Core
WEB_REPORT_SESSION_LIMIT = 500andAI_HIGHLIGHT_LIMIT = 15constants intermstory/web.pyto replace inline magic numbersLIMIT 500, replaced with single parameterized query usingWEB_REPORT_SESSION_LIMITAI_HIGHLIGHT_LIMITinstead of inlineLIMIT 15session_windowmetadata to returned dict withlimit,returned,total, andtruncatedfieldsUI
.session-window-noticewith.visiblemodifier classrole="status"andaria-live="polite"attributessessionWindow.truncatedis trueTests
session_windowassertion totest_get_web_data_empty_dbtest_get_web_data_does_not_truncate_unfiltered_sessions_at_30to verify 31 sessions return without truncationtest_generate_and_open_reportto verify notice HTML and JavaScript wiringtest_swarm_audit_fixesto derive overflow fixture fromWEB_REPORT_SESSION_LIMITinstead of hardcoding 1000/1005Architecture / Flow
flowchart LR A[get_web_data] --> B[Query Sessions] B --> C[Apply WEB_REPORT_SESSION_LIMIT] C --> D[Build session_window Metadata] D --> E[Return to Frontend] E --> F{truncated?} F -->|Yes| G[Show Notice] F -->|No| H[Hide Notice]Fixes
Testing
python -m pytest tests/test_web.py -q --timeout=60— 9 passeduvx ruff check termstory/web.py— passedgit diff --check— passedNotes
upstream/main@1996a2fand resolved conflict by retaining upstream's unified 500-session cap throughWEB_REPORT_SESSION_LIMITwhile preserving the PR's parameterized query,session_windowmetadata, truncation notice, and regression coverage.