Repository navigation
ADFA-3305: Read compressed templates from documentation version 3 - #2164
Conversation
Written as a join of Tooltips and TooltipCategories, the tooltip query made SQLite scan all of Tooltips - 46,128 rows, 13 MB - for every tooltip shown, because documentation.db carries no ANALYZE statistics (EXPLAIN QUERY PLAN: "SCAN T"). Looking the category's id up first lets the UNIQUE (categoryId, tag) index find the row: 6.2 ms -> 0.004 ms per lookup on a desktop against the real database, same rows. IN rather than =, so two categories sharing a name still come back as duplicates, which getTooltip reports as before. The buttons query still scans TooltipButtons (5.4 MB); that needs an index in the database, which this change cannot add. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…nds them From review of PR #2160: - TooltipQueryTest bound the query's arguments itself, so it would pass whatever order getTooltip used, and this PR reversed that order. The SQL and its binding now live in queryTooltip, in TooltipQuery.kt, which getTooltip calls with named arguments and the test calls too. Swapping the binding fails two of the three tests. - QUERY_TOOLTIP is marked @VisibleForTesting: only the plan test reads it. It is still a public static field in bytecode, as it was inside TooltipManager, and so still in the plugin-api jar. - Drops @ConscryptMode(OFF). The Conscrypt leak it guarded against broke a TLS test in common; idetooltips has none. A file of its own rather than top-level code in ToolTipManager.kt: ktlint leaves ToolTipManager.kt unformatted while its only top-level declaration, object TooltipManager, does not match the file name. A second declaration there makes Spotless reformat all of the file's ~380 changed lines, burying this change. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
No behavior change. The four helpers that write and delete a plugin's tooltips move, unchanged, to PluginTooltipTables.kt behind two internal functions, writePluginTooltips and removePluginTooltips, so tests can run them against an in-memory database without a Context. The manager now maps each PluginTooltipEntry to its final text and resolved button URIs before handing them over, inside the same transaction as before. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
documentation.db version 3 (OfflineDocumentationTools' migrate_tooltip_schema.py) stores the same tooltips in 5.2 MB less: - Tooltips is a WITHOUT ROWID table keyed on (categoryId, tag) holding only the id, so the separate UNIQUE (categoryId, tag) index, a second copy of every tag, is gone. The text is in TooltipTexts, by id. - TooltipButtons is keyed on (tooltipId, buttonNumberId), so a tooltip's buttons are one key range in display order instead of a scan and sort of the whole table. Each distinct description and uri is stored once, in TooltipButtonDescriptions and TooltipButtonUris. queryTooltip joins TooltipTexts by id, and the new queryTooltipButtons joins the two lookup tables. Tooltips has no rowid any more; it always equalled id, so IDETooltipItem.rowId, which the debug dialog and the feedback email show, now carries the id. Both readers refuse a database declaring a version below 3, through DatabaseVersionResolver.requireKeyedTooltips, with a message naming the version, rather than fail on whichever old column a query meets. Tests run both queries against the version 3 DDL, including their query plans (key searches, no scan, no sort), and the version check for versions 2, 4 and none. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
PluginTooltipTables.kt now writes the version 3 tooltip tables: - A tooltip's text goes into TooltipTexts first, whose AUTOINCREMENT id then keys the Tooltips row. The id is the one that insert returns: Tooltips is WITHOUT ROWID, and SQLite does not record its inserts as a rowid, so an insert into it returns the previous insert's rowid, not anything of its own. - Each button's description and uri are looked up in, or added to, TooltipButtonDescriptions and TooltipButtonUris, 500 strings per query, so text a built-in button already uses is shared. - A plugin's rows are deleted by category through subqueries, which also removes the old one-variable-per-tooltip IN list, and text no button names any more is deleted after every write and removal. - Every insert throws on failure. db.insert returned -1 instead, so a failed insert left a tooltip without its buttons, or buttons on tooltip -1, and the transaction committed anyway. - Both entry points refuse a database declaring a version below 3 before writing anything. Tests run against the version 3 DDL with one built-in tooltip: what a plugin writes reads back through the app's queries; new ids are never reused; a repeated tag keeps the later entry; text is shared and cleaned up; removing every plugin leaves the tables exactly as they were; 1,200 tooltips write and remove; a version 2 database is refused untouched; and a failed insert into any of the six tables throws and rolls back. plugin-manager's tests now use Robolectric, already in the version catalog, for SQLiteDatabase. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
No behaviour change. The query, the row-count check and the mapping of the row and its buttons to an IDETooltipItem move out of TooltipManager.getTooltip into readTooltip beside queryTooltip, so tests can run them without initializing TooltipManager, which reads the installed app's Environment. Spotless reformatted the moved `when`. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
getTooltip reads the installed documentation.db or, when it is newer, the /sdcard/Download debug override, and the log said neither: a "declares documentation version 2" line left a developer guessing which file to replace. The log now names the source and its path. Review finding R2163-03. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
No test ran getTooltip's row mapping, though this branch renumbered its columns and made rowId the id: with the summary read from the detail column, all 12 idetooltips tests still passed. A tooltip whose id, summary, detail and buttons all differ is now read back into the exact item, so that mutant, a swapped detail and a wrong rowId each fail it. Also covers the missing-tag and duplicate-category exceptions. Review finding R2163-04. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
docs/documentation-database.md said the database is extracted on install and update; InstallationViewModel extracts it at first-time setup only, so an update keeps the installed file, and a version 2 one then shows no tooltips (accepted for ADFA-3305). It also said docdb-studio edits "this same documentation.db" and validates TooltipButtons.uri: the master it edits stays in version 2, and migrate_tooltip_schema.py converts a copy for shipping, so docdb-studio and the docs/docdb scripts use the version 2 columns. The debug-override notes now say a version 2 file shows no tooltips, and the ToolTipManager entry names readTooltip. idetooltips/README.md cited a Room IDETooltipDatabase that does not exist; the module reads documentation.db with raw SQLite. Review finding R2163-05. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
From documentation version 3 every Templates row is Brotli-compressed against the database's CompressionDictionary, like a brotli Content row. No column says so: the release build compresses every template, and nothing on the device writes Templates. - DatabaseTemplateLoader passes each row's bytes through a decode before Pebble sees them, so included templates are covered too. A decode failure is logged with the template's name and the database's path, and fails the page with a message that names the template but no file, since that message reaches HTTP responses. - DocumentationContentSource decodes with the active database's codec when the database declares major >= 3 (expectsCompressedTemplates). Below 3, and with no version, templates are read as plain UTF-8 as before: an app update never replaces the installed database, so a 2.x file keeps rendering its templated pages. The answer is read once per database, before the engine runs, and cleared on every swap. - An internal constructor takes the decode, so :common's tests, which have no brotli4j desktop native, can stand one in; CompressedTemplateTest in :app covers the real decode. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@coderabbitai review |
❌ Action failedReview failed.
|
|
@coderabbitai full review |
❌ Action failedReview failed. |
Independent Claude review (head 2bb16a6)The change does what its body says. There is one way into a template,
The findings are a test gap on the line the PR rewrote, and stale text in the docs and the body.
R2164-01. The PR rewrote
Read as ISO-8859-1, a U+00A0 (bytes C2 A0) renders as "Â" plus a no-break space on every page that includes one of those 631 templates, and the suite stays green. Fix: in R2164-02. Line 85 says R2164-03. The body has two figures that don't match the databases:
Fix: name the file each figure comes from, or use these. Checked and sound
🤖 Generated with Claude Code |
640 of the 3.0.0 build's 1,126 templates hold non-ASCII text, most of them U+00A0, but every template in the tests was ASCII: reading the decoded bytes as ISO-8859-1 at DatabaseTemplateLoader.kt:105 left all 66 :common documentation tests and 62 :app localWebServer tests passing, while a real page would show "A-circumflex" before each no-break space. Six tests now render text with U+00A0, U+2014 and U+2026, plain and decoded: two in the loader's tests, two through DocumentationContentSource with a stand-in decode, and two in :app through the real Brotli decode. The page tests compare bytes. All six fail with ISO_8859_1 at line 105 (the one line both paths share), and the four page tests also fail with the rendered output encoded as ISO_8859_1. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
…ated documentation-database.md said CompressionDictionary was "trained once ... never retrained", but OfflineDocumentationTools' remint_dictionary.py (on its main since ADFA-5153) retrains it and recompresses every brotli Content row, and from version 3 every template too (ODT #48). Now that templates depend on the dictionary, the rule a reader needs is that a re-mint is safe only when it carries every row and template across. The content source's entry called the templated rows "the Kotlin doc set's pages"; a 2.x database also templates the i/ pages, and the 3.0.0 build the a/ pages. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3
Response to the reviewAll three findings are addressed (2bb16a6..f2178d7).
🤖 Generated with Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git rev-parse --short HEAD
fd -t f '^(DatabaseTemplateLoader|DocumentationContentSource|DatabaseTemplateLoaderTest|DocumentationContentSourceTest|CompressedTemplateTest)\.kt$' | while IFS= read -r file; do
printf '\n--- %s ---\n' "$file"
ast-grep outline "$file" || true
rg -n -C 8 'UTF_8|ISO_8859_1|00[Aa]0|2014|2026|non.ASCII|[—…]|assertArrayEquals|assertContentEquals' "$file" || true
done
printf '\n--- Documentation ---\n'
sed -n '65,115p' docs/documentation-database.md
printf '\n--- Current PR description ---\n'
gh pr view 2164 --repo appdevforall/CodeOnTheGo --json headRefOid,body --jq '{headRefOid,body}'Length of output: 52367
I did not run the tests or mutation checks. The passing results remain your reported results, not an independent verification. The failed full-review response gives no cause. It does not establish that CodeRabbit is unavailable for this PR. I have not triggered another review. Thanks for clarifying the dictionary re-minting requirement. ✏️ Learnings added
🧠 Learnings used
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Merge-order note for #2162 and #2164. Whichever of the two merges second needs a small fix, or it won't compile.
Found while building a local integration of #2163, #2164, #2161 and #2162 for the ADFA-3305 phone test. With these two changes it compiles, and its pages passed the phone check with the item-B database. 🤖 Generated with Claude Code |
#2164 gives renderNamed a database parameter. Taking it here, with the same lines, keeps renderJsTopic compiling whichever of the two lands second.
|
Merge note: #2162 and #2164 conflict in
Keep the field, and build it with #2164's decoding constructor: // A compressed row is decoded with the active database's codec, the one its Content rows use.
private val templateLoader =
DatabaseTemplateLoader({ database }) { database, body ->
if (templatesCompressed(database)) decodeTemplate(codec(database), body) else body
}
private val pebbleEngine =
PebbleEngine
.Builder()
.loader(templateLoader)
.maxRenderedSize(MAX_RENDERED_CHARS)
.build()
|
…2162) * ADFA-6618: Render a Help+Manual topic's jstopics file from its page Help+Manual's in-page navigation loads jstopics/X.js, a JavaScript copy of topic X. The documentation pipeline is to store each topic once, as the JSON its page X.html renders from through a Pebble template, and drop these copies (OfflineDocumentationTools PR #43). So a lookup that finds no row for a path of the form [dir/]jstopics/X.js now renders the helpmanual-jstopic.peb template with the JSON of [dir/]X.html and serves it as text/javascript. A stored jstopics row still wins, and the miss stays a 404 when the page is not a single templated row, has no row, or the database has no such template. Tests: six new cases in DocumentationContentSourceTest. Breaking each guard in turn fails the test named for it: no rule, no template check, no templateId check, a regex that accepts subfolders, the wrong MIME type. :common has 211 tests passing, and spotlessCheck passes. docs/documentation-database.md describes the rule beside the other template rendering. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3 * ADFA-6618: Answer the PR #2162 review From the independent review of PR #2162: - R2162-01: the rule ran inside the raw-path lookup, so a raw form that rendered kept a stored jstopics row under the percent-decoded form from being found. lookupRequestPath now tries both stored forms first and renders only after both miss; lookup() reads stored rows only. - R2162-02: a duplicated page row is reported as Ambiguous, as the page itself is, rather than as a quiet 404. - R2162-03: tests for a Brotli-compressed page (bytes marked brotli that are not fail the decode instead of rendering) and for a page split across rows. - R2162-05: the template check uses DatabaseTemplateLoader's resourceExists, and reading and decoding a row is one helper shared by both callers. - R2162-06: the rule applies only to a page rendered through helpmanual-topic.peb, so another doc set's jstopics path stays a 404. - R2162-07: the docs no longer call templated rows only the Kotlin doc set's pages, and name both Help+Manual templates. R2162-04 belongs to OfflineDocumentationTools PR #43. Tests: DocumentationContentSourceTest goes through lookupRequestPath, the lookup both transports make; five new cases. Breaking each new guard fails the test named for it: the rule before the decoded lookup, a quiet duplicate, any page template, no template check, no Brotli decode, the first chunk only. :common has 216 tests passing, and spotlessCheck passes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3 * ADFA-6618: Report a duplicated topic page under the page's path From CodeRabbit's review of PR #2162: a duplicated i/X.html row made a request for i/jstopics/X.js return Ambiguous under the jstopics path. WebServer quotes that path in its "Corrupt database" answer so that a bug report carries a query reproducing the duplicate, and the jstopics path has no rows at all. lookupJsTopic now returns the page's path with an Ambiguous result; other results keep the requested path. The duplicated-page test now also checks the reported path, and fails with the path left as the jstopics one. :common has 216 tests passing, and spotlessCheck passes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3 * ADFA-6618: Test the decoded-form and Brotli renders From the independent review of 36c2838 and ea8976a: - R2162-08: a test renders a jstopics file from a page found only under the percent-decoded path (i/a b.html for i/jstopics/a%20b.js). - R2162-09: lookupRequestPath's KDoc names the jstopics rule it now owns, and that a duplicated page is reported under the page's path. And CodeRabbit's nitpick on R2162-03: no test decoded a valid Brotli row through DocumentationContentSource, so a decode that always failed passed every test. :common's tests can't load brotli4j's desktop native, so BrotliContentSourceTest in :app serves a stored Brotli row and renders a jstopics file from a Brotli page. Breaking each fails the test named for it: dropping the decoded-form render fails the new :common test; a decode that always throws fails both BrotliContentSourceTest cases. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3 * ADFA-6618: Pass the database to renderNamed, as #2164 does #2164 gives renderNamed a database parameter. Taking it here, with the same lines, keeps renderJsTopic compiling whichever of the two lands second. --------- Co-authored-by: Claude Opus 5.5 <[email protected]> Co-authored-by: Hal Eisen <[email protected]>
|
Warning Review limit reachedOnly developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Next included review available in 3 minutes. View limit details
Comment |
…ssed-templates # Conflicts: # common/src/main/java/com/itsaky/androidide/documentation/DocumentationContentSource.kt # common/src/main/java/com/itsaky/androidide/utils/DatabaseVersionResolver.kt # docs/documentation-database.md
ADFA-3305
Reads Brotli-compressed templates from documentation version 3. Builds on #2163, so it targets
fix/ADFA-3305-tooltip-schema, and ships with it: version 3.0.0 now means keyed tooltips and compressed templates, written by OfflineDocumentationTools' ship-time steps.Why
The ADFA-3305 conversion of the
a/class pages adds about 1,000 templates (12 MB as text). Measured on the full database with every template compressed and the dictionary re-minted, it saves 7.47 MB of file and 7.55 MB of.br. The app reads templates as plain text today.Change
DatabaseTemplateLoaderpasses each row's bytes through a decode before Pebble sees them, soincluded templates are covered too. A decode failure is logged with the template's name and the database's path, and fails the page withCannot decode template '<name>', a message with no file name, since it reaches HTTP responses.DocumentationContentSourcedecodes with the active database's codec when it declares major >= 3 (expectsCompressedTemplates). Below 3, and with no version, templates are read as plain UTF-8, as today: an app update never replaces the installed database, and the installed 2.x file renders 3,490 pages through templates. The answer is read once per database, before the engine runs, and cleared on every swap.Templates.:common's tests have no brotli4j desktop native.docs/documentation-database.mdupdated.Tests
CompressedTemplateTest(new,:app, real Brotli), 7 tests::common:DatabaseTemplateLoaderTest+3 (decode applied, failure wrapped by name, failure logged with the database) andDocumentationContentSourceTest+7.WebServerTestbookshelf test pins the no-version plain path.:commondocumentation 66,:applocalWebServer 62,:idetooltips12,:plugin-managerall passing.spotlessCheckclean.:app:assembleV8Debugbuilds.Not covered
Device check
On 2026-10-19 (Pixel 6 Pro, Android 17), this build with the ship-time database from OfflineDocumentationTools'
steps (3.0.0, compressed templates) as the debug override:
through compressed templates and includes; ten pages gave the same text as on the earlier schema-3 database.
Load times from the app's log: 11-451 ms; R.styleable (still HTML) 1,992 ms.
errors. Swapping back brought the 3.0.0 pages back.
:common(56) and:app(19) tests pass there.Release
Merge with #2163, and ship in the same switch as the 3.0.0 database.
🤖 Generated with Claude Code
https://claude.ai/code/session_018CMZQuXT3GT7nEct2w4tg3