danvics/docling-studio
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix(clean-code): English mode strings, SRP extraction, rename getter

Browse source 
- Replace French mode strings (configurer/verifier/preparer) with English
  equivalents (configure/verify/prepare) in StudioPage.vue and tests
- Extract _build_conversion_options, _run_conversion, _finalize_analysis
  from _run_analysis_inner to respect Single Responsibility Principle
- Rename _get_default_converter to _ensure_default_converter to reflect
  its lazy-init side effect

Closes #136, closes #137, closes #138
This commit is contained in:
Pier-Jean Malandrino 2026-04-10 12:57:31 +02:00
parent a82db25e59
commit 3d199cb783
6 changed files with 101 additions and 83 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
document-parser/infra/local_converter.py
View file

@ -114,7 +114,7 @@ def _build_docling_converter(options: ConversionOptions) -> DoclingConverter:
)
def _get_default_converter() -> DoclingConverter:
def _ensure_default_converter() -> DoclingConverter:
global _default_converter
if _default_converter is None:
try:
@ -127,7 +127,7 @@ def _get_default_converter() -> DoclingConverter:
def _select_converter(options: ConversionOptions) -> DoclingConverter:
if options.is_default():
return _get_default_converter()
return _ensure_default_converter()
return _build_docling_converter(options)

114
document-parser/services/analysis_service.py
View file

@ -268,6 +268,67 @@ class AnalysisService:
job_id, file_path, filename, pipeline_options, chunking_options
)
def _build_conversion_options(self, pipeline_options: dict | None) -> ConversionOptions:
"""Build ConversionOptions, applying default table_mode if not specified."""
opts_dict = pipeline_options or {}
if "table_mode" not in opts_dict:
opts_dict = {**opts_dict, "table_mode": self._config.default_table_mode}
return ConversionOptions(**opts_dict)
async def _run_conversion(
self,
job_id: str,
file_path: str,
options: ConversionOptions,
) -> ConversionResult | None:
"""Run batched or single conversion. Returns None if the job was deleted mid-batch."""
total_pages = _count_pdf_pages(file_path)
batch_size = self._config.batch_page_size
if batch_size > 0 and total_pages > batch_size:
return await self._run_batched_conversion(
job_id, file_path, options, total_pages, batch_size
)
return await asyncio.wait_for(
self._converter.convert(file_path, options),
timeout=self._conversion_timeout,
)
async def _finalize_analysis(
self,
job_id: str,
result: ConversionResult,
chunking_options: dict | None,
) -> None:
"""Serialize results, optionally chunk, mark job completed, update page count."""
pages_json = json.dumps([asdict(p) for p in result.pages])
chunks_json = None
if chunking_options and self._chunker and result.document_json:
chunk_opts = ChunkingOptions(**chunking_options)
chunks = await self._chunker.chunk(result.document_json, chunk_opts)
chunks_json = json.dumps([_chunk_to_dict(c) for c in chunks])
logger.info("Chunking produced %d chunks for job %s", len(chunks), job_id)
# Re-read the job so we don't lose progress_current/progress_total
# written to the DB during batched conversion.
job = await self._analysis_repo.find_by_id(job_id)
if not job:
return
job.mark_completed(
markdown=result.content_markdown,
html=result.content_html,
pages_json=pages_json,
document_json=result.document_json,
chunks_json=chunks_json,
)
await self._analysis_repo.update_status(job)
if result.page_count:
await self._document_repo.update_page_count(job.document_id, result.page_count)
logger.info("Analysis completed: %s (%d pages)", job_id, result.page_count)
async def _run_analysis_inner(
self,
job_id: str,
@ -287,55 +348,12 @@ class AnalysisService:
await self._analysis_repo.update_status(job)
logger.info("Analysis started: %s (file: %s)", job_id, filename)
opts_dict = pipeline_options or {}
if "table_mode" not in opts_dict:
opts_dict = {**opts_dict, "table_mode": self._config.default_table_mode}
options = ConversionOptions(**opts_dict)
options = self._build_conversion_options(pipeline_options)
result = await self._run_conversion(job_id, file_path, options)
if result is None:
return # job was deleted mid-batch
total_pages = _count_pdf_pages(file_path)
batch_size = self._config.batch_page_size
if batch_size > 0 and total_pages > batch_size:
result = await self._run_batched_conversion(
job_id,
file_path,
options,
total_pages,
batch_size,
)
if result is None:
return # job was deleted mid-batch
else:
result = await asyncio.wait_for(
self._converter.convert(file_path, options),
timeout=self._conversion_timeout,
)
pages_json = json.dumps([asdict(p) for p in result.pages])
chunks_json = None
if chunking_options and self._chunker and result.document_json:
chunk_opts = ChunkingOptions(**chunking_options)
chunks = await self._chunker.chunk(result.document_json, chunk_opts)
chunks_json = json.dumps([_chunk_to_dict(c) for c in chunks])
logger.info("Chunking produced %d chunks for job %s", len(chunks), job_id)
# Re-read the job so we don't lose progress_current/progress_total
# written to the DB during batched conversion.
job = await self._analysis_repo.find_by_id(job_id) or job
job.mark_completed(
markdown=result.content_markdown,
html=result.content_html,
pages_json=pages_json,
document_json=result.document_json,
chunks_json=chunks_json,
)
await self._analysis_repo.update_status(job)
if result.page_count:
await self._document_repo.update_page_count(job.document_id, result.page_count)
logger.info("Analysis completed: %s (%d pages)", job_id, result.page_count)
await self._finalize_analysis(job_id, result, chunking_options)
except TimeoutError:
logger.error("Analysis timed out after %ds: %s", self._conversion_timeout, job_id)

18
document-parser/tests/test_pipeline_options.py
View file

@ -146,7 +146,7 @@ class TestBuildConverter:
class TestConvertDocumentRouting:
"""Verify convert_document uses default converter for default opts, custom otherwise."""
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_default_converter_with_all_defaults(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -164,7 +164,7 @@ class TestConvertDocumentRouting:
mock_get_default.assert_called_once()
mock_build.assert_not_called()
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_ocr_disabled(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -182,7 +182,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once()
mock_get_default.assert_not_called()
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_table_mode_fast(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -200,7 +200,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once_with(opts)
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_code_enrichment_on(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -218,7 +218,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once_with(opts)
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_formula_enrichment_on(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -235,7 +235,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once()
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_picture_options_on(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -252,7 +252,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once()
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_generate_images_on(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -269,7 +269,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once()
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_uses_custom_converter_when_images_scale_changed(self, mock_build, mock_get_default):
mock_conv = MagicMock()
@ -287,7 +287,7 @@ class TestConvertDocumentRouting:
mock_build.assert_called_once_with(opts)
@patch("infra.local_converter._get_default_converter")
@patch("infra.local_converter._ensure_default_converter")
@patch("infra.local_converter._build_docling_converter")
def test_forwards_all_options_to_build_converter(self, mock_build, mock_get_default):
mock_conv = MagicMock()

4
document-parser/tests/test_robustness.py
View file

@ -159,7 +159,7 @@ import infra.local_converter as lc_mod # noqa: E402
build_converter = lc_mod._build_docling_converter
convert_sync = lc_mod._convert_sync
get_default_converter = lc_mod._get_default_converter
get_default_converter = lc_mod._ensure_default_converter
# ---------------------------------------------------------------------------
# C1 — document_timeout in PdfPipelineOptions
@ -317,7 +317,7 @@ class TestConvertSyncLimits:
class TestGetDefaultConverterReset:
"""Verify _get_default_converter resets on failure and retries on next call."""
"""Verify _ensure_default_converter resets on failure and retries on next call."""
@pytest.fixture(autouse=True)
def reset_default_converter(self):

14
frontend/src/features/history/navigation.test.ts
View file

@ -125,7 +125,7 @@ describe('History → Studio navigation', () => {
})
describe('Full restore flow (store-level integration)', () => {
it('restores completed analysis: selects analysis + document + verifier mode', async () => {
it('restores completed analysis: selects analysis + document + verify mode', async () => {
const { fetchAnalysis } = await import('../analysis/api')
fetchAnalysis.mockResolvedValue({
id: 'a1',
@ -147,12 +147,12 @@ describe('History → Studio navigation', () => {
docStore.select(analysis.documentId)
expect(docStore.selectedId).toBe('d1')
// Mode would be set to 'verifier' since status is COMPLETED
const mode = analysis.status === 'COMPLETED' ? 'verifier' : 'configurer'
expect(mode).toBe('verifier')
// Mode would be set to 'verify' since status is COMPLETED
const mode = analysis.status === 'COMPLETED' ? 'verify' : 'configure'
expect(mode).toBe('verify')
})
it('restores failed analysis: selects analysis + document, stays in configurer mode', async () => {
it('restores failed analysis: selects analysis + document, stays in configure mode', async () => {
const { fetchAnalysis } = await import('../analysis/api')
fetchAnalysis.mockResolvedValue({
id: 'a2',
@ -171,8 +171,8 @@ describe('History → Studio navigation', () => {
docStore.select(analysis.documentId)
expect(docStore.selectedId).toBe('d2')
const mode = analysis.status === 'COMPLETED' ? 'verifier' : 'configurer'
expect(mode).toBe('configurer')
const mode = analysis.status === 'COMPLETED' ? 'verify' : 'configure'
expect(mode).toBe('configure')
})
it('handles missing analysis gracefully', async () => {

30
frontend/src/pages/StudioPage.vue
View file

@ -23,8 +23,8 @@
<button
class="toggle-btn"
data-e2e="toggle-btn"
:class="{ active: mode === 'configurer' }"
@click="mode = 'configurer'"
:class="{ active: mode === 'configure' }"
@click="mode = 'configure'"
>
<svg class="toggle-icon" viewBox="0 0 20 20" fill="currentColor">
<path
@ -38,8 +38,8 @@
<button
class="toggle-btn"
data-e2e="toggle-btn"
:class="{ active: mode === 'verifier' }"
@click="mode = 'verifier'"
:class="{ active: mode === 'verify' }"
@click="mode = 'verify'"
:disabled="!analysisStore.currentAnalysis"
>
<svg class="toggle-icon" viewBox="0 0 20 20" fill="currentColor">
@ -55,8 +55,8 @@
v-if="chunkingEnabled"
class="toggle-btn"
data-e2e="toggle-btn"
:class="{ active: mode === 'preparer' }"
@click="mode = 'preparer'"
:class="{ active: mode === 'prepare' }"
@click="mode = 'prepare'"
:disabled="!analysisStore.currentAnalysis"
>
<svg class="toggle-icon" viewBox="0 0 20 20" fill="currentColor">
@ -82,7 +82,7 @@
data-e2e="run-btn"
:disabled="analysisStore.running"
@click="runAnalysis"
v-if="mode === 'configurer'"
v-if="mode === 'configure'"
>
<div v-if="analysisStore.running" class="spinner-sm" />
<svg v-else viewBox="0 0 20 20" fill="currentColor" class="btn-icon">
@ -226,7 +226,7 @@
@load="onPdfImageLoad"
/>
<BboxOverlay
v-if="(visualMode || mode === 'preparer') && hasAnalysisResults"
v-if="(visualMode || mode === 'prepare') && hasAnalysisResults"
ref="bboxOverlayRef"
:image-el="pdfImageRef"
:page-data="currentPageData"
@ -246,7 +246,7 @@
<!-- Right: Config or Results panel -->
<div class="right-panel" :style="{ width: rightPanelWidth + 'px' }">
<!-- CONFIGURER MODE -->
<div v-if="mode === 'configurer'" class="config-panel" data-e2e="config-panel">
<div v-if="mode === 'configure'" class="config-panel" data-e2e="config-panel">
<div class="config-section">
<label class="config-label">
{{ t('config.model') }}
@ -429,7 +429,7 @@
</div>
<!-- VERIFIER MODE -->
<div v-if="mode === 'verifier'" class="verify-panel">
<div v-if="mode === 'verify'" class="verify-panel">
<ResultTabs
:current-page="currentPage"
:highlighted-index="highlightedElementIndex"
@ -438,7 +438,7 @@
</div>
<!-- PREPARER MODE (feature-flipped) -->
<div v-if="mode === 'preparer' && chunkingEnabled" class="prepare-panel">
<div v-if="mode === 'prepare' && chunkingEnabled" class="prepare-panel">
<ChunkPanel
:current-page="currentPage"
@highlight-bboxes="highlightedChunkBboxes = $event"
@ -470,7 +470,7 @@ const analysisStore = useAnalysisStore()
const { t } = useI18n()
const chunkingEnabled = useFeatureFlag('chunking')
const mode = ref('configurer')
const mode = ref('configure')
const currentPage = ref(1)
const visualMode = ref(false)
const highlightedElementIndex = ref(-1)
@ -573,12 +573,12 @@ watch(currentPage, () => {
highlightedChunkBboxes.value = []
})
// Auto-switch to verifier when analysis completes + refresh document data (pageCount)
// Auto-switch to verify when analysis completes + refresh document data (pageCount)
watch(
() => analysisStore.currentAnalysis?.status,
(status) => {
if (status === 'COMPLETED') {
mode.value = 'verifier'
mode.value = 'verify'
documentStore.load()
}
},
@ -596,7 +596,7 @@ onMounted(async () => {
if (analysis) {
documentStore.select(analysis.documentId)
if (analysis.status === 'COMPLETED') {
mode.value = 'verifier'
mode.value = 'verify'
}
}
// Clean query param from URL