Skip to content

ADFA-3879 resilient LastChange table#1302

Open
hal-eisen-adfa wants to merge 4 commits into
stagefrom
ADFA-3879-resilient-wholedb-missing-from-LastChange
Open

ADFA-3879 resilient LastChange table#1302
hal-eisen-adfa wants to merge 4 commits into
stagefrom
ADFA-3879-resilient-wholedb-missing-from-LastChange

Conversation

@hal-eisen-adfa
Copy link
Copy Markdown
Collaborator

Handle missing documentation.db LastChange wholedb record by fallback
Never crash regardless of LastChange table

…; never crash regardless of LastChange table
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Review Change Stack

📝 Walkthrough

Release Notes

Features & Improvements

  • Resilient database versioning: Introduced DatabaseVersionResolver utility to centralize and safeguard database version resolution from the LastChange table
  • Fallback handling for missing wholedb record: If the preferred wholedb documentation set record is missing, the resolver automatically falls back to the most recent LastChange entry across any documentation set
  • Crash-proof versioning: Application now gracefully handles missing or malformed LastChange tables by returning "Version Unknown" instead of crashing
  • Centralized version resolution: Replaced manual SQL queries in WebServer.logDatabaseLastChanged() and TooltipManager.getTooltip() with the new resolver
  • Enhanced test coverage: Added 6 comprehensive test cases covering normal operation, fallback scenarios, NULL handling, and edge cases (empty/missing tables)

Technical Details

  • Formatter supports displaying changeTime, optional documentationSet, and optional who fields
  • Handles NULL values gracefully in both primary and fallback query paths
  • Logging of fallback events and exceptions for debugging and monitoring
  • Wrapped version resolution calls in try-catch blocks for defensive programming

Risks & Best Practice Concerns

  • ⚠️ SQL injection potential: Uses SQLiteDatabase.rawQuery() which is generally discouraged despite hard-coded queries; consider migrating to parameterized query() method for consistency with Android best practices
  • ⚠️ Log level inconsistency: Missing wholedb record logged as ERROR level; consider using WARN since fallback is functional and expected in some deployments
  • ⚠️ Unrelated changes included: PR includes tooltip screenshot testing infrastructure (TooltipDebugDialogScreenshotTest, TooltipScreenshotHostActivity) that appears tangential to the main resilience objective
  • ⚠️ No migration strategy: No documented approach for handling existing production databases that lack the wholedb record or proper LastChange schema

Walkthrough

This PR centralizes database version/last-change resolution by introducing a new DatabaseVersionResolver utility, integrating it into TooltipManager and WebServer to replace direct SQL queries, and adds debug-only screenshot testing infrastructure for tooltip UI validation.

Changes

Database Version Resolution and Integration

Layer / File(s) Summary
DatabaseVersionResolver implementation
common/src/main/java/com/itsaky/androidide/utils/DatabaseVersionResolver.kt
resolveDatabaseVersion(db) queries the LastChange table for wholedb records, falls back to the latest row if missing, and formatVersion() constructs version strings from non-null changeTime, documentationSet, and who values.
DatabaseVersionResolver test coverage
common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt
Tests verify resolver behavior across scenarios: returning wholedb rows when present, falling back to the latest row when wholedb is absent, returning VERSION_UNKNOWN for missing/empty tables, and correctly handling nullable who columns in formatted output.
TooltipManager migration
idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt
getTooltip() now calls DatabaseVersionResolver.resolveDatabaseVersion() instead of directly querying the LastChange table; the old SQL query constant is removed.
WebServer migration
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
logDatabaseLastChanged() now calls DatabaseVersionResolver.resolveDatabaseVersion() instead of executing a cursor-based query.

Tooltip Screenshot Testing

Layer / File(s) Summary
Tooltip screenshot host and manifest
app/src/debug/java/com/itsaky/androidide/idetooltips/TooltipScreenshotHostActivity.kt, app/src/debug/AndroidManifest.xml
Introduces a debug-only empty AppCompatActivity to anchor tooltip popups for testing, declared as an exported activity with IDE theme.
Tooltip screenshot instrumentation test
app/src/androidTest/kotlin/com/itsaky/androidide/idetooltips/TooltipDebugDialogScreenshotTest.kt
Instrumentation test that launches the host activity, shows an IDE tooltip anchored to a button, clicks its info icon to open a debug dialog, and captures a screenshot validated as non-empty.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • itsaky-adfa
  • Daniel-ADFA
  • jomen-adfa

Poem

🐰 A resolver hops through databases with grace,
No more SQL tangled in odd places!
Tooltips now screenshot their debug-info views,
While tests ensure nothing the resolver might lose.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: introducing resilient handling for the LastChange table, which aligns with the changeset's focus on handling missing wholedb records and preventing crashes.
Description check ✅ Passed The description is directly related to the changeset, explaining the purpose of handling missing LastChange wholedb records with fallback logic and crash prevention.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ADFA-3879-resilient-wholedb-missing-from-LastChange

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt (1)

42-52: ⚡ Quick win

Consider adding a test case for multiple wholedb records.

The current test verifies that a single wholedb record is returned correctly. To ensure the query returns the latest record when multiple wholedb entries exist (especially after adding ORDER BY changeTime DESC as suggested), consider adding a test case that inserts multiple wholedb records with different timestamps and verifies the most recent one is returned.

🧪 Example test case
`@Test`
fun returnsLatestWholedbRow_whenMultiplePresent() {
	createTable()
	insertRow("wholedb", "2026-05-01 10:00:00", "alice")
	insertRow("wholedb", "2026-05-09 02:00:20", "hal")
	insertRow("wholedb", "2026-05-03 14:30:00", "bob")

	assertEquals(
		"2026-05-09 02:00:20 hal",
		DatabaseVersionResolver.resolveDatabaseVersion(db),
	)
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt`
around lines 42 - 52, Add a new test in DatabaseVersionResolverTest (e.g.,
returnsLatestWholedbRow_whenMultiplePresent) that uses createTable() and
insertRow() to insert multiple "wholedb" rows with different changeTime values,
then assert DatabaseVersionResolver.resolveDatabaseVersion(db) returns the most
recent record (timestamp + author); reference the existing
returnsWholedbRow_whenPresent test for structure and ensure the inserted
timestamps exercise the ORDER BY changeTime DESC behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/src/debug/AndroidManifest.xml`:
- Line 7: Remove the android:exported="true" attribute from the debug-only test
activity declaration (the activity launched by ActivityScenario.launch()) so the
activity is not exported to other apps; edit the debug AndroidManifest entry for
that test activity and either delete the android:exported attribute or set it to
false to restrict visibility.

In `@common/src/main/java/com/itsaky/androidide/utils/DatabaseVersionResolver.kt`:
- Around line 12-17: The QUERY_WHOLEDB SQL constant currently selects LIMIT 1
from LastChange without ordering, which can return non-deterministic rows;
update the QUERY_WHOLEDB constant to include an ORDER BY changeTime DESC
(matching QUERY_FALLBACK_LATEST) so the latest wholedb record is
deterministically returned when using LIMIT 1.

---

Nitpick comments:
In
`@common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt`:
- Around line 42-52: Add a new test in DatabaseVersionResolverTest (e.g.,
returnsLatestWholedbRow_whenMultiplePresent) that uses createTable() and
insertRow() to insert multiple "wholedb" rows with different changeTime values,
then assert DatabaseVersionResolver.resolveDatabaseVersion(db) returns the most
recent record (timestamp + author); reference the existing
returnsWholedbRow_whenPresent test for structure and ensure the inserted
timestamps exercise the ORDER BY changeTime DESC behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 016fc624-892c-440f-a64b-02ad7f47ea54

📥 Commits

Reviewing files that changed from the base of the PR and between b85bbc2 and 634b800.

📒 Files selected for processing (7)
  • app/src/androidTest/kotlin/com/itsaky/androidide/idetooltips/TooltipDebugDialogScreenshotTest.kt
  • app/src/debug/AndroidManifest.xml
  • app/src/debug/java/com/itsaky/androidide/idetooltips/TooltipScreenshotHostActivity.kt
  • app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
  • common/src/androidTest/java/com/itsaky/androidide/utils/DatabaseVersionResolverTest.kt
  • common/src/main/java/com/itsaky/androidide/utils/DatabaseVersionResolver.kt
  • idetooltips/src/main/java/com/itsaky/androidide/idetooltips/ToolTipManager.kt

Comment thread app/src/debug/AndroidManifest.xml Outdated
@hal-eisen-adfa hal-eisen-adfa requested a review from a team May 13, 2026 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants